search

ksahlin / strobealign

Aligns short reads using dynamic seed size with strobemers
MIT License
128 stars 16 forks source link

Fix secondary alignments not having CIGAR strings #290

Closed marcelm closed 1 year ago

marcelm commented 1 year ago

This was a regression introduced in 51eab56e07dbbb88c67772e7cafa4b777646946f: After best_sam_aln = std::move(sam_aln), sam_aln should not be used, but we did use it when appending it to the alignments vector.

The actual bugfix is in 6fb332684574a76569bbd29fcce0f787eb4aae51. This PR also sets SEQ and QUAL to * for secondary alignments as recommended by the SAM spec (see #212). (I verified that this is what BWA-MEM does as well.)

Closes #289 Closes #66

ksahlin commented 1 year ago

Great! Does this PR (ca4febf) also fix the off-by-one error in setting -N reported in issue https://github.com/ksahlin/strobealign/issues/66?

marcelm commented 1 year ago

Ah, I think I just forgot to close #66: I think the off-by-one error has already been fixed by #235 (07dc9e42a3b0e73541eb3ae332c277b34a3cd1a5) by just passing map_params.max_secondary + 1 to the function instead of map_params.max_secondary. The variable name was still misleading at that point and commit ca4febf now makes it so that the variable actually contains the maximum number of secondary hits (this makes align_SE consistent with align_PE consistent in that regard).

marcelm commented 1 year ago

I forgot to push a commit updating the changelog. I just pushed that to main directly.