Implementation of a SAMRecord comparator that matches samtools' queryname sort order.#1600
Conversation
|
Codecov Report❌ Patch coverage is
❌ Your changes status has failed because you have indirect coverage changes. Learn more about Unexpected Coverage Changes and reasons for indirect coverage changes. Additional details and impacted files@@ Coverage Diff @@
## master #1600 +/- ##
===============================================
+ Coverage 69.838% 69.852% +0.014%
- Complexity 9653 9664 +11
===============================================
Files 703 704 +1
Lines 37647 37681 +34
Branches 6114 6121 +7
===============================================
+ Hits 26292 26321 +29
- Misses 8904 8906 +2
- Partials 2451 2454 +3
🚀 New features to boost your workflow:
|
|
Always best to be I second place: #1601 |
|
Do you need to fall back on the first and second of pair flags as samtools does? https://github.com/samtools/samtools/blob/bdc5bb81c11f2ab25ea97351213cb87b33857c4d/bam_sort.c#L1798 |
|
@nh13 I guess it depends on whether you want to match samtools 100% or implement the "natural" sort order which doesn't prescribe how ties are broken. I don't think I have a strong opinion so long as e.g. first of pair sorts before second of pair, and secondary/supplementary records sort after primary |
|
I think the ideal way to make this work, but which would be a PITA to retrofit would be something akin to fgbio's SamOrder, which defines more than the |
|
Ah, the eternal problem #766. I was assuming that awkwardly working around this was the impetus for the last pr? I think we have a few options none of which are trivial. An alternative would be to try to make some sort of meta queryname sorter which tries to figure out the subsort when it's reading a file. That shouldn't be hard I would think, just try both sort orders and if one passes it's valid. I can't remember if there's any issue with stateful comparators but I think it wouldn't be a problem to have one that figures out which it is and then remembers it. We should write the SS field in either case... |
|
@lbergelson I don't think samtools writes the I think the easiest first step would be to have HTSJDK accept either as being queryname sorted - HTSJDK already has that Then the question becomes ... if it can accept either as valid, is there any reason not to switch to the samtools style as the default output for |
|
The main reason not to switch the default immediately is that a lot of people used mixed pipeline where some of the tools are on older versions. Switching would break them until they update all parts of the pipeline. Does samtools have trouble reading htsjdk queryname bams? I'm not sure the problem is symmetrical. |
|
I don't believe there are issues with samtools reading picard/htsjdk queryname sorted files, but it's not something I do regularly. I think samtools tends more towards trusting/assuming the user provided acceptable input and not validating. |
Reworks the original draft (#1600) into a form ready to build on: - Replace the long-based numeric comparison, which overflowed on numeric runs of ~19+ digits, with a digit-by-digit port of htslib's strnum_cmp. It is now a faithful match to samtools for numeric runs of any length. - Expose the read-name comparison as a public static compareNatural(String,String), mirroring the parent's compareReadNames, so it is usable without instantiating the comparator. - Use explicit ASCII '0'-'9' digit testing (read names are ASCII per the SAM spec) rather than the Unicode-aware Character.isDigit. - Expand the single spec-example test into 12 focused tests: value-not-lexical ordering, leading-zero tie-breaks, the large-number (overflow) regression, empty/prefix/equal names, antisymmetry, and record-level tests that exercise the inherited pair/strand tie-breaking. This is the standalone comparator only; wiring it to a SAM SS:queryname:natural sub-sort tag is deliberately left to a follow-up, since htsjdk has no sub-sort concept yet and that is a broader, cross-cutting design.
Research into the actual reference implementation (samtools bam_sort.c strnum_cmp)
showed current samtools ignores leading-zero counts entirely: numeric runs with
the same value compare equal regardless of leading zeros ("8" == "08" == "008"),
relying on a stable sort for their relative order. The earlier port ordered
"more leading zeros first", matching older htslib and a non-normative example
(the SAM spec itself defines no natural-order algorithm and contains no such
example). Since the goal is to match samtools, drop the leading-zero tiebreak.
Tests updated accordingly: leading-zero-only differences now assert equality,
and the ordered-list test uses names with a single unambiguous ordering.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds a public comparator for natural ordering of SAM query names, comparing digit runs by numeric value without overflow and preserving inherited SAM record tie-breaking. Tests cover string ordering, large values, comparator properties, and unmapped record integration. ChangesNatural query-name ordering
Estimated code review effort: 3 (Moderate) | ~20 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Revisited the comparator and made a couple of changes to align it more precisely with samtools:
Fuller integration with the SAM |
4 participants
@nh13, @lbergelson I put this together mostly for fun - it's a comparator that will sort read names the same way samtools does. I'm not really sure what the best way to integrate this is in HTSJDK though. Thoughts?
Summary by CodeRabbit
New Features
Tests