Test ContainerIO.calculateSliceOffsetsAndSizes() and fix the slice size calculation #1326
Conversation
| } | ||
| final Slice lastSlice = container.slices[container.slices.length - 1]; | ||
| lastSlice.offset = container.landmarks[container.landmarks.length - 1]; | ||
| lastSlice.size = container.containerByteSize - lastSlice.offset; |
There was a problem hiding this comment.
this was the error
Codecov Report
@@ Coverage Diff @@
## master #1326 +/- ##
===============================================
+ Coverage 67.804% 67.822% +0.018%
- Complexity 8252 8253 +1
===============================================
Files 562 562
Lines 33641 33644 +3
Branches 5639 5640 +1
===============================================
+ Hits 22810 22818 +8
+ Misses 8658 8654 -4
+ Partials 2173 2172 -1
|
| for (int i = 0; i < container.slices.length - 1; i++) { | ||
| static void calculateSliceOffsetsAndSizes(final Container container) { | ||
| if (container.slices.length == 0) { | ||
| return; |
There was a problem hiding this comment.
Is a legitimate case where this can happen ?
There was a problem hiding this comment.
What happens if this is called with a container with a slice with an unmapped reference context ?
There was a problem hiding this comment.
Q1: Yes - if this is the BAM File Header container.
Q2: none of these values relate to alignment, so there should be no difference there.
| // the container's header size = 9000 | ||
| // Slice[0].offset = 9000 | ||
| // Slice[0].size = 109000 - 9000 = 100000 | ||
| // Slice[1].offset = 109000 |
| // this Container consists of: | ||
| // a header of size 1234 bytes | ||
| // a Slice of size 789 bytes | ||
| // a Slice of size 5555 bytes |
There was a problem hiding this comment.
This needs to also have a test for the most common case we expect to see, which is a Container with a single, single-reference slice. And also a test with an unmapped reference context slice.
There was a problem hiding this comment.
I'll add one for single slice.
Not sure about changing Ref Context because it's irrelevant to this test.
src/test/java/htsjdk/samtools/cram/structure/ContainerTest.java
Outdated
Show resolved
Hide resolved
src/test/java/htsjdk/samtools/cram/structure/ContainerTest.java
Outdated
Show resolved
Hide resolved
…ze calculation move ContainerIO.calculateSliceOffsetsAndSizes() to - Container.populateSlicesAndIndexingParameters()
jmthibault79
force-pushed
the
jt_slice_size_fix
branch
from
63a30b5
to
cc5e617
Compare
- added comments clarifying the real situation - see samtools/hts-specs#396 - see samtools/hts-specs#398
* Revert #1326 because it was correct before - added comments clarifying the real situation - see samtools/hts-specs#396 - see samtools/hts-specs#398 - comments and rename CRAIEntry.sliceByteOffset - blockCount comments
Description
Problem:
calculateSliceOffsetsAndSizes() was calculating Slice.size incorrectly.
Impact:
Slices created via ContainerIO.readContainer() would produce incorrect sliceByteSize values for their CRAI entries.
Checklist