CRAM: formalize Slice Type with an enum #1274

jmthibault79 · 2019-02-11T20:25:10Z

Description

Add a ReferenceContext member to Slice and Container, based on the discussion at samtools/hts-specs#370 and the related spec update at samtools/hts-specs#372.

Checklist

Code compiles correctly
New tests covering changes and new functionality
All tests passing
Extended the README / documentation, if necessary
Is not backward compatible (breaks binary or source compatibility)

jmthibault79 · 2019-02-11T20:25:52Z

src/main/java/htsjdk/samtools/CRAMBAIIndexer.java

@@ -135,7 +134,7 @@ public void processContainer(final Container container, final ValidationStringen
                AlignmentSpan unmappedSpan = refSet.remove(SAMRecord.NO_ALIGNMENT_REFERENCE_INDEX);
                for (final int refId : new TreeSet<>(refSet.keySet())) {
                    final AlignmentSpan span = refSet.get(refId);
-                    fakeSlice.sequenceId = refId;
+                    fakeSlice.setSequenceId(refId);


also sets the SliceType, to ensure they stay in sync

codecov-io · 2019-02-11T20:37:40Z

Codecov Report

Merging #1274 into master will increase coverage by 0.027%.
The diff coverage is 84.528%.

@@               Coverage Diff               @@
##              master     #1274       +/-   ##
===============================================
+ Coverage     67.728%   67.755%   +0.027%     
- Complexity      8225      8236       +11     
===============================================
  Files            560       562        +2     
  Lines          33493     33540       +47     
  Branches        5637      5638        +1     
===============================================
+ Hits           22684     22725       +41     
- Misses          8632      8633        +1     
- Partials        2177      2182        +5

Impacted Files	Coverage Δ	Complexity Δ
src/main/java/htsjdk/samtools/cram/CRAIEntry.java	`71.053% <100%> (ø)`	`17 <0> (ø)`	⬇️
src/main/java/htsjdk/samtools/CRAMIterator.java	`81.752% <100%> (+0.409%)`	`31 <0> (-1)`	⬇️
...a/htsjdk/samtools/cram/build/ContainerFactory.java	`96.774% <100%> (-0.196%)`	`5 <0> (-1)`
src/main/java/htsjdk/samtools/cram/CRAIIndex.java	`82.716% <100%> (-0.211%)`	`27 <1> (ø)`
...jdk/samtools/cram/build/Cram2SamRecordFactory.java	`93.162% <100%> (ø)`	`29 <0> (ø)`	⬇️
...oding/reader/MultiRefSliceAlignmentSpanReader.java	`100% <100%> (ø)`	`5 <2> (ø)`	⬇️
...amtools/cram/encoding/writer/CramRecordWriter.java	`89.231% <100%> (ø)`	`26 <0> (ø)`	⬇️
...amtools/cram/encoding/reader/CramRecordReader.java	`85.256% <100%> (ø)`	`26 <0> (ø)`	⬇️
...c/main/java/htsjdk/samtools/cram/build/CramIO.java	`75% <100%> (-0.51%)`	`20 <0> (ø)`
...htsjdk/samtools/cram/ref/ReferenceContextType.java	`100% <100%> (ø)`	`1 <1> (?)`
... and 11 more

src/main/java/htsjdk/samtools/CRAMContainerStreamWriter.java

jmthibault79 · 2019-02-11T20:44:24Z

src/main/java/htsjdk/samtools/CRAMIterator.java

+                prevSeqId = Slice.MULTI_REFERENCE;
+                break;
+            default:
+                if (prevSeqId != container.getSequenceId()) {


removed the prevSeqId < 0 check: since we know that the slice type is single-ref, we also know that the current sequence ID here is 0+. So it will differ from any negative values.

jmthibault79 · 2019-02-11T20:49:28Z

src/main/java/htsjdk/samtools/cram/structure/Slice.java

    // as defined in the specs:
-    public int sequenceId = -1;
+    private int sequenceId = -1;


can't modify this without also modifying type

cmnbroad

Some initial comments/questions; haven't looked at the tests yet.

src/main/java/htsjdk/samtools/cram/structure/slice/SliceType.java

src/main/java/htsjdk/samtools/cram/structure/Container.java

src/main/java/htsjdk/samtools/cram/structure/Slice.java

src/main/java/htsjdk/samtools/CRAMContainerStreamWriter.java

src/main/java/htsjdk/samtools/cram/structure/Container.java

cmnbroad

A few more comments - see what you think and maybe we can discuss when we meet.

src/main/java/htsjdk/samtools/CRAMBAIIndexer.java

src/main/java/htsjdk/samtools/CRAMContainerStreamWriter.java

src/main/java/htsjdk/samtools/cram/structure/Container.java

jmthibault79 · 2019-02-20T22:16:47Z

src/main/java/htsjdk/samtools/CRAMIterator.java

+        switch (containerContext.getType()) {
+            case UNMAPPED:
+                referenceBases = new byte[]{};
+                prevSeqId = SAMRecord.NO_ALIGNMENT_REFERENCE_INDEX;


future candidate for turning into a ReferenceContext

jmthibault79 · 2019-02-20T22:17:26Z

src/main/java/htsjdk/samtools/CRAMContainerStreamWriter.java

@@ -208,7 +201,7 @@ protected boolean shouldFlushContainer(final SAMRecord nextRecord) {
            return true;
        }

-        if (refSeqIndex == Slice.MULTI_REFERENCE) {
+        if (refSeqIndex == Slice.REF_ID_MULTIPLE) {


refSeqIndex is a future candidate for ReferenceContext

jmthibault79 · 2019-02-20T22:19:12Z

src/main/java/htsjdk/samtools/cram/build/ContainerParser.java

@@ -59,60 +60,55 @@ public ContainerParser(final SAMFileHeader samFileHeader) {
        return records;
    }

-    public Map<Integer, AlignmentSpan> getReferences(final Container container, final ValidationStringency validationStringency) {


someday we will merge a PR that collapses all these stupid things. I may even have such a thing open?

Yes: that's over at #1276

jmthibault79 · 2019-02-20T22:20:24Z

src/main/java/htsjdk/samtools/cram/encoding/reader/CramRecordReader.java

                cramRecord.sequenceId = refIdCodec.readData();
            } else {
-                cramRecord.sequenceId = refId;
+                cramRecord.sequenceId = refContext.getSequenceIdOrSentinel();


watch out: can be unmapped here

Perhaps at some point CramRecord should have a ReferenceContext instead, or along with, a sequence ID.

jmthibault79 · 2019-02-21T20:08:36Z

src/main/java/htsjdk/samtools/cram/build/CramIO.java

        container.blockCount = 1;
        container.blocks = new Block[]{block};
        container.landmarks = new int[0];
        container.slices = new Slice[0];
-        container.alignmentSpan = Slice.NO_ALIGNMENT_SPAN;


these fields are now initialized to these values

jmthibault79 · 2019-02-21T20:23:41Z

src/main/java/htsjdk/samtools/cram/structure/ContainerHeaderIO.java

        final byte[] peek = new byte[4];
        try {
            int character = inputStream.read();
            if (character == -1)
-                return false;
+                return readContainerHeader(new ByteArrayInputStream((major >= 3 ? CramIO.ZERO_F_EOF_MARKER : CramIO.ZERO_B_EOF_MARKER)));


This was what the caller did when it received a false from this method.

Mildly not crazy about the recursion, but this change would make it directly recursive instead of indirectly, allow the otherwise unused method above to be deleted:

Suggested change

return readContainerHeader(new ByteArrayInputStream((major >= 3 ? CramIO.ZERO_F_EOF_MARKER : CramIO.ZERO_B_EOF_MARKER)));

return readContainerHeader(major, new ByteArrayInputStream((major >= 3 ? CramIO.ZERO_F_EOF_MARKER : CramIO.ZERO_B_EOF_MARKER)));

jmthibault79 · 2019-02-21T20:24:27Z

src/main/java/htsjdk/samtools/cram/structure/ContainerIO.java

@@ -56,13 +56,7 @@ private static Container readContainer(final int major, final InputStream inputS
     * @return a new {@link Container} object with container header values filled out but empty body (no slices and blocks).
     */
    public static Container readContainerHeader(final int major, final InputStream inputStream) {
-        final Container container = new Container();


logic moved inside ContainerHeaderIO.readContainerHeader()

cmnbroad

Looks like a step in the right direction. Some comments and suggestions.

src/main/java/htsjdk/samtools/cram/ref/ReferenceContext.java

src/main/java/htsjdk/samtools/cram/structure/ContainerHeaderIO.java

cmnbroad · 2019-02-26T15:46:14Z

src/main/java/htsjdk/samtools/cram/structure/ContainerHeaderIO.java

        final byte[] peek = new byte[4];
        try {
            int character = inputStream.read();
            if (character == -1)
-                return false;
+                return readContainerHeader(new ByteArrayInputStream((major >= 3 ? CramIO.ZERO_F_EOF_MARKER : CramIO.ZERO_B_EOF_MARKER)));


Comment for another day: Its a bit confusing to have methods called readContainerHeader and writeContainerHeader that return/accept Containers (but only partially populated ?), while Container has a header field, but the type is CramCompressionHeader.

src/main/java/htsjdk/samtools/cram/build/ContainerFactory.java

cmnbroad · 2019-02-26T15:57:05Z

src/main/java/htsjdk/samtools/cram/encoding/reader/CramRecordReader.java

                cramRecord.sequenceId = refIdCodec.readData();
            } else {
-                cramRecord.sequenceId = refId;
+                cramRecord.sequenceId = refContext.getSequenceIdOrSentinel();


Perhaps at some point CramRecord should have a ReferenceContext instead, or along with, a sequence ID.

cmnbroad · 2019-02-26T16:10:40Z

src/main/java/htsjdk/samtools/cram/structure/ContainerHeaderIO.java

        final byte[] peek = new byte[4];
        try {
            int character = inputStream.read();
            if (character == -1)
-                return false;
+                return readContainerHeader(new ByteArrayInputStream((major >= 3 ? CramIO.ZERO_F_EOF_MARKER : CramIO.ZERO_B_EOF_MARKER)));


Mildly not crazy about the recursion, but this change would make it directly recursive instead of indirectly, allow the otherwise unused method above to be deleted:

Suggested change

return readContainerHeader(new ByteArrayInputStream((major >= 3 ? CramIO.ZERO_F_EOF_MARKER : CramIO.ZERO_B_EOF_MARKER)));

return readContainerHeader(major, new ByteArrayInputStream((major >= 3 ? CramIO.ZERO_F_EOF_MARKER : CramIO.ZERO_B_EOF_MARKER)));

src/main/java/htsjdk/samtools/cram/ref/ReferenceContext.java

- replace Slice.REF_ID_MULTIPLE/UNMAPPED with ReferenceContext constants - refs -> referenceBases - initializeFromRecords() - init Slice Aln Start and Span - don't return null - endPlusOne - rename getSequenceIdOrSentinel() to getSerializableId() and add toString() - rm unused readContainerHeader - test addiitonal Slice + Container fields

cmnbroad

Aside from some naming comments, the code changes themselves look pretty good. Mostly requests to update some of the tests.

Also, as it stands, the ReferenceContextType enum isn't really used anywhere except tests. I'm not sure if thats because we haven't yet changed all the code to use ReferenceContexts, or if its because its redundant with ReferenceContext. Seems harmless to leave it for now, just wondering if we still need it, or if ReferenceContext makes it redundant.

src/main/java/htsjdk/samtools/cram/ref/ReferenceContext.java

src/main/java/htsjdk/samtools/cram/build/ContainerFactory.java

src/main/java/htsjdk/samtools/cram/encoding/reader/MultiRefSliceAlignmentSpanReader.java

src/main/java/htsjdk/samtools/cram/structure/Container.java

src/test/java/htsjdk/samtools/cram/build/ContainerFactoryTest.java

src/test/java/htsjdk/samtools/cram/ref/ReferenceContextTest.java

src/main/java/htsjdk/samtools/cram/ref/ReferenceContext.java

src/test/java/htsjdk/samtools/cram/ref/ReferenceContextTest.java

Co-Authored-By: jmthibault79 <thibault@broadinstitute.org>

cmnbroad

Looks pretty good, thx for the test changes. Still a couple of comments on additional code consolidation in the tests and test utils. If we don't resolve those as part of this PR, I'd request that we do so in the next PR, or even a separate PR specifically for that purpose.

cmnbroad · 2019-03-01T14:50:55Z

src/test/java/htsjdk/samtools/cram/structure/SliceTests.java

-            Assert.assertTrue(slice.isMappedSingleRef());
-        }
+        Assert.assertEquals(slice.nofRecords, expectedRecordCount);
+        Assert.assertEquals(slice.bases, expectedBaseCount);
    }

    private CramCompressionRecord createRecord(final int index,


This createRecord method is nearly identical to createMappedRecord in CramCompressionRecordUtil. getUnplacedRecords is (completely?) identical to 'getUnmappedRecordsinCramCompressionRecordUtil`. etc.

cmnbroad · 2019-03-01T15:13:44Z

src/test/java/htsjdk/samtools/cram/structure/ContainerTest.java

-        Assert.assertEquals(container.sequenceId, slice.sequenceId);
+        Assert.assertTrue(container.getReferenceContext().isMappedSingleRef());
+        Assert.assertEquals(container.getReferenceContext().getType(), ReferenceContextType.SINGLE_REFERENCE_TYPE);
+        Assert.assertEquals(container.getReferenceContext(), slice.getReferenceContext());


Many of the blocks of asserts in these tests could be replaced with one of the ContainerFactoryTest.assertContainerState methods. Not sure if SliceTests.assertSliceState could be reuseable as well.

jmthibault79 commented Feb 11, 2019

View reviewed changes

src/main/java/htsjdk/samtools/CRAMContainerStreamWriter.java Show resolved Hide resolved

jmthibault79 commented Feb 11, 2019

View reviewed changes

jmthibault79 requested review from cmnbroad and lbergelson February 12, 2019 17:55

jmthibault79 mentioned this pull request Feb 12, 2019

CRAM indexing tests and associated fixes and updates #1276

Merged

5 tasks

cmnbroad requested changes Feb 12, 2019

View reviewed changes

jmthibault79 force-pushed the jt_states_enum branch from 2311d9a to a888718 Compare February 13, 2019 15:53

cmnbroad reviewed Feb 14, 2019

View reviewed changes

jmthibault79 force-pushed the jt_states_enum branch from b558805 to b51fb3d Compare February 20, 2019 21:46

jmthibault79 commented Feb 20, 2019

View reviewed changes

jmthibault79 force-pushed the jt_states_enum branch from 1c87ef3 to 20f9eda Compare February 21, 2019 18:36

jmthibault79 commented Feb 21, 2019

View reviewed changes

jmthibault79 force-pushed the jt_states_enum branch from 20f9eda to d5b815f Compare February 21, 2019 20:52

yfarjoun assigned cmnbroad Feb 25, 2019

cmnbroad reviewed Feb 26, 2019

View reviewed changes

jmthibault79 force-pushed the jt_states_enum branch from 9e286aa to c537ec9 Compare February 27, 2019 20:50

jmthibault79 force-pushed the jt_states_enum branch from c537ec9 to 71f2729 Compare February 27, 2019 20:56

jmthibault79 added 2 commits February 27, 2019 16:17

split the sentinels tests

0f40603

cleanup

d742bb9

cmnbroad requested changes Feb 28, 2019

View reviewed changes

fix signature

9a4af51

jmthibault79 and others added 6 commits February 28, 2019 15:01

rm todo

357089c

import

7754683

use ReferenceContext constants

9491b4a

ReferenceContext constant name consistency

8d00791

refactor ContainerFactoryTest and ContainerParserTest

13ddb8c

Refactor ReferenceContextTest.java

2e2fa48

Co-Authored-By: jmthibault79 <thibault@broadinstitute.org>

jmthibault79 force-pushed the jt_states_enum branch from 53bf87f to 2e2fa48 Compare February 28, 2019 22:01

cmnbroad approved these changes Mar 1, 2019

View reviewed changes

cmnbroad added the cram label Mar 1, 2019

jmthibault79 merged commit 37b2e87 into master Mar 1, 2019

jmthibault79 deleted the jt_states_enum branch March 1, 2019 16:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CRAM: formalize Slice Type with an enum #1274

CRAM: formalize Slice Type with an enum #1274

jmthibault79 commented Feb 11, 2019 •

edited

Loading

jmthibault79 Feb 11, 2019

codecov-io commented Feb 11, 2019 •

edited

Loading

jmthibault79 Feb 11, 2019

jmthibault79 Feb 11, 2019

cmnbroad left a comment

cmnbroad left a comment

jmthibault79 Feb 20, 2019

jmthibault79 Feb 20, 2019

jmthibault79 Feb 20, 2019

jmthibault79 Feb 21, 2019

jmthibault79 Feb 20, 2019

cmnbroad Feb 26, 2019

jmthibault79 Feb 21, 2019

jmthibault79 Feb 21, 2019

cmnbroad Feb 26, 2019

jmthibault79 Feb 21, 2019

cmnbroad left a comment •

edited

Loading

cmnbroad Feb 26, 2019

cmnbroad Feb 26, 2019

cmnbroad Feb 26, 2019

cmnbroad left a comment

cmnbroad left a comment

cmnbroad Mar 1, 2019

cmnbroad Mar 1, 2019

	return readContainerHeader(new ByteArrayInputStream((major >= 3 ? CramIO.ZERO_F_EOF_MARKER : CramIO.ZERO_B_EOF_MARKER)));
	return readContainerHeader(major, new ByteArrayInputStream((major >= 3 ? CramIO.ZERO_F_EOF_MARKER : CramIO.ZERO_B_EOF_MARKER)));

CRAM: formalize Slice Type with an enum #1274

CRAM: formalize Slice Type with an enum #1274

Conversation

jmthibault79 commented Feb 11, 2019 • edited Loading

Description

Checklist

Choose a reason for hiding this comment

codecov-io commented Feb 11, 2019 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cmnbroad left a comment

Choose a reason for hiding this comment

cmnbroad left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cmnbroad left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cmnbroad left a comment

Choose a reason for hiding this comment

cmnbroad left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmthibault79 commented Feb 11, 2019 •

edited

Loading

codecov-io commented Feb 11, 2019 •

edited

Loading

cmnbroad left a comment •

edited

Loading