One possible solution to cigarLen>65535 (DON'T MERGE; for discussion only) #938
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This approach was initially proposed in this thread (solution 1): https://sourceforge.net/p/samtools/mailman/message/30672431/ It is reusing the bin field to store the higher 16 bits of cigarLen. Importantly, this change is only applied to BAM files. Once the BAMRecord is read into memory by new htsjdk, all the fields should be correctly populated.
It is silly to send a PR not compilable. Even for discussion purposes.
Codecov Report
@@ Coverage Diff @@
## master #938 +/- ##
===============================================
- Coverage 65.348% 65.336% -0.012%
- Complexity 7321 7323 +2
===============================================
Files 528 528
Lines 31975 31993 +18
Branches 5469 5472 +3
===============================================
+ Hits 20895 20903 +8
- Misses 8921 8930 +9
- Partials 2159 2160 +1
|
|
PS: I submitted this PR because in an internal discussion, @jkbonfield thought this solution still has advantages: it crashes older tools sometimes and is simple to implement. I have PRs to htslib and hts-specs on the solution @yfarjoun and I have agreed on, but I am not familiar with htsjdk enough to implement that in short time. To me, both solutions (this PR and samtools/htslib#560) are fine as long as we can reach a consensus. |
|
I am closing this PR now, in favor of #1003. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
First of all, this PR is not intended to be merged. It is only meant to show code changes for a solution to CIGARs with >65535 operations. The solution was first proposed here (solution 1). It is not the solution @yfarjoun and I have agreed on, but it probably requires the least code change. The other solution needs to add a few hundred lines of code to htsjdk across multiple files.
You can see how the approach works from the change to BAMRecordCodec.encode(). When cigarLen is too large, we write indexingBin, cigarLen and flags differently. The decoder sets the correct values for these fields once the BAMRecord is read to memory.
./gradlew testreported "all tests passed" in the end, but this doesn't tell us whether/how my changes work with a new BAM with long cigars.Checklist