Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Emit M instead of =/X CIGAR operations by default #261

Merged
merged 5 commits into from
Apr 11, 2023
Merged

Emit M instead of =/X CIGAR operations by default #261

merged 5 commits into from
Apr 11, 2023

Conversation

marcelm
Copy link
Collaborator

@marcelm marcelm commented Apr 11, 2023

I needed to do some refactoring so that CIGARs are internally always represented as Cigar objects and only converted to their string representation at the very last moment when the alignments are serialized to SAM format. This makes it easy to optionally convert them to use M instead of =/X operations since the logic for that needs to be in only one place.

I chose --m-op as the name for the command-line option for now, but I’m not super happy with it. Bowtie2 and minimap2 have an --eqx switch and the opposite behavior: M by default and =/X is optional. Perhaps we should change the default as well for better compatibility? Also, that would avoid the spurious variant calling issue as long as htslib is not fixed.

@ksahlin
Copy link
Owner

ksahlin commented Apr 11, 2023

Great! And I agree that we should change default to be M and have the flag --eqx to write =/Xcigar strings.

I will restart the SNV/indel calling benchmark once this is merged.

@marcelm marcelm changed the title Add --m-op option for emitting M instead of =/X CIGAR operations Emit M instead of =/X CIGAR operations by default Apr 11, 2023
@marcelm
Copy link
Collaborator Author

marcelm commented Apr 11, 2023

Alright, updated to use M by default, and the previous behavior can be restored with --eqx.

@marcelm marcelm merged commit f041502 into main Apr 11, 2023
@marcelm marcelm deleted the meqx branch April 11, 2023 20:12
@ksahlin
Copy link
Owner

ksahlin commented Apr 23, 2023

The evaluation of this commit was posted here: #245 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants