-
Notifications
You must be signed in to change notification settings - Fork 242
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
CRAM: revert #1326 and fix tests and comments #1341
Conversation
- added comments clarifying the real situation - see samtools/hts-specs#396 - see samtools/hts-specs#398
#1326 was based on a misunderstanding of values in the CRAM Container Header. samtools/hts-specs#398 is the companion PR for the spec. |
@@ -159,7 +159,7 @@ void recordMetaData(final Slice slice) { | |||
unAlignedRecords += slice.unmappedReadsCount; | |||
} | |||
|
|||
final long start = slice.byteOffsetFromContainer; | |||
final long start = slice.byteOffsetFromCompressionHeaderStart; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's a mouthful but it's correct
} | ||
|
||
final Slice lastSlice = slices[lastSliceIndex]; | ||
lastSlice.index = lastSliceIndex; | ||
lastSlice.byteOffsetFromContainer = landmarks[lastSliceIndex]; | ||
|
||
// calculate a "final landmark" indicating the byte offset of the end of the container |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no. containerBlocksByteSize
is already equal to that "final landmark".
This is the section that reverts #1326
Codecov Report
@@ Coverage Diff @@
## master #1341 +/- ##
===============================================
- Coverage 67.848% 67.846% -0.003%
Complexity 8273 8273
===============================================
Files 562 562
Lines 33678 33675 -3
Branches 5653 5653
===============================================
- Hits 22850 22847 -3
Misses 8653 8653
Partials 2175 2175
|
final ReferenceContext refContext = new ReferenceContext(0); | ||
|
||
final Container container = new Container(refContext); | ||
container.containerByteSize = slice0size; | ||
container.containerBlocksByteSize = compressionHeaderSize + slice0size; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is the change to the test (as well the similar line below)
// a Slice of size 6262 bytes | ||
|
||
final int containerHeaderSize = 1234; | ||
final int compressionHeaderSize = 7552; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the meaningful changes here are below, in createOneSliceContainer()
/ createTwoSliceContainer()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a couple of minor changes requested.
Description
Please explain the changes you made here.
Explain the motivation for making this change. What existing problem does the pull request solve?
Checklist