Fix samtools merge --template-coordinate#2062
Conversation
|
Looks like there are detected memory leaks. I have not used |
|
Looks like when I added a test case for |
|
@daviesrob any chance you could review this given your past review on this feature? Also, how hard would it be to make a patch release for this bug fix? |
There's a bug in `samtools merge --template-coordinate' that causes the output not to be correctly sorted. Here, we re-sort via fgbio SortBam to ensure the output is properly sorted. fgbio SortBam can be removed when the fix to the samtools bug is released. See: samtools/samtools#2062
There's a bug in `samtools merge --template-coordinate' that causes the output not to be correctly sorted. Here, we re-sort via fgbio SortBam to ensure the output is properly sorted. fgbio SortBam can be removed when the fix to the samtools bug is released. See: samtools/samtools#2062
There's a bug in `samtools merge --template-coordinate' that causes the output not to be correctly sorted. Here, we re-sort via fgbio SortBam to ensure the output is properly sorted. fgbio SortBam can be removed when the fix to the samtools bug is released. See: samtools/samtools#2062
There's a bug in `samtools merge --template-coordinate' that causes the output not to be correctly sorted. Here, we re-sort via fgbio SortBam to ensure the output is properly sorted. fgbio SortBam can be removed when the fix to the samtools bug is released. See: samtools/samtools#2062
daviesrob
left a comment
There was a problem hiding this comment.
This looks OK apart from the issue noted. You could possibly shrink the test files a bit by removing the unneeded @SQ lines. Including the ones mentioned in XA and SA tags, you should only need these:
chr1
chr2
chr3
chr4
chr5
chr6
chr7
chr8
chr9
chr10
chr11
chr12
chr13
chr14
chr15
chr16
chr17
chr18
chr19
chr20
chr21
chr22
chrX
chrUn_KI270749v1
chr1_GL383519v1_alt
chr1_KI270761v1_alt
chr1_KI270765v1_alt
chr1_KI270892v1_alt
chr3_GL383526v1_alt
chr3_KI270778v1_alt
chr4_KI270790v1_alt
chr5_GL383532v1_alt
chr6_GL000250v2_alt
chr6_GL000251v2_alt
chr6_GL000252v2_alt
chr6_GL000253v2_alt
chr6_GL000254v2_alt
chr6_GL000255v2_alt
chr6_GL000256v2_alt
chr6_KI270801v1_alt
chr11_KI270829v1_alt
chr11_KI270903v1_alt
chr13_KI270843v1_alt
chr15_KI270848v1_alt
chr16_KI270728v1_random
chr16_KI270853v1_alt
chr17_KI270857v1_alt
chr19_GL949746v1_alt
chr19_GL949747v2_alt
chr19_GL949748v2_alt
chr19_GL949749v2_alt
chr19_GL949750v2_alt
chr19_GL949751v2_alt
chr19_GL949753v2_alt
chr19_KI270938v1_alt
chr22_KI270875v1_alt
chrUn_JTFH01000763v1_decoy
Removing the others would shrink the test files by over 200K each. You might be able to get away with removing some of the alignment records as well, as only the first one appears to move as a result of the bug fix...
f54dd24 to
cbeba40
Compare
cbeba40 to
1f4aa85
Compare
|
I've simplified the test SAMs, as well as added back the lines to clean up the keys. |
|
Thanks, this is looking better. One issue I've just noticed is that on There are still some memory leaks on |
… mem_fail Thanks to @daviesrob
|
@daviesrob thanks for the suggested improvement, done! |
|
Thanks for the updates, now all merged. |
|
Thank-you for all the suggestions and review! |
|
@daviesrob any chance for a release with this fix included? I'd like to update pipelines to use a released version :) |
There's a bug in `samtools merge --template-coordinate' that causes the output not to be correctly sorted. Here, we re-sort via fgbio SortBam to ensure the output is properly sorted. fgbio SortBam can be removed when the fix to the samtools bug is released. See: samtools/samtools#2062
Cause: the wrong heap was being tested.
I added a test case from the original reported issue here: nf-core/fastquorum#52.