CRAM queryAlignmentStart/queryMate fix. #1164

cmnbroad · 2018-07-12T18:08:46Z

Fixes an issue discovered as part of debugging #1065. The CRAM implementation of queryAlignmentStart (which is in turn used by queryMate) currently returns all reads in a container that start after the requested alignment start. This PR pulls out the BAMStartingAtFilterIterator into a top-level class so it can be reused by the CRAM implementation to filter, and then stop iterating after, all reads with the desired start have been returned.

codecov-io · 2018-09-06T18:54:24Z

Codecov Report

Merging #1164 into master will increase coverage by 0.016%.
The diff coverage is 87.879%.

@@               Coverage Diff               @@
##              master     #1164       +/-   ##
===============================================
+ Coverage     67.863%   67.879%   +0.016%     
- Complexity      8289      8294        +5     
===============================================
  Files            563       564        +1     
  Lines          33718     33729       +11     
  Branches        5659      5659               
===============================================
+ Hits           22882     22895       +13     
+ Misses          8656      8655        -1     
+ Partials        2180      2179        -1

Impacted Files	Coverage Δ	Complexity Δ
src/main/java/htsjdk/samtools/BAMFileReader.java	`68.466% <ø> (-0.199%)`	`52 <0> (ø)`
...a/htsjdk/samtools/BAMStartingAtIteratorFilter.java	`86.667% <86.667%> (ø)`	`5 <5> (?)`
src/main/java/htsjdk/samtools/CRAMFileReader.java	`77.295% <88.889%> (+1.274%)`	`53 <0> (ø)`	⬇️

src/test/java/htsjdk/samtools/CRAMIndexQueryTest.java

yfarjoun · 2018-09-06T22:46:15Z

src/main/java/htsjdk/samtools/CRAMFileReader.java


+        /**
+         * Subclasses must call this method in their constructors AFTER construction of this class is complete.


could you not have a "abstract callInitialize()" method in the base class and then call that method from the base constructor? that would force an subclass to implement this method and read this comment...otherwise I'm not sure it will be noticed....

I agree that this is error prone but at least it's a private class, so it's likely a future subclasser will read this comment before writing code.

I don't see why this class still has CRAMIntervalIteratorBase(final QueryInterval[] queries, final boolean contained, final long[] coordinates) as coordinates is ignored. Removing that constructor will also be a way to tell callers that they must call this method, which you could rename to something like setCoordinates().

Yes, that constructor appears to be redundant. Removed. Not sure I understand how that helps with the requirement to call initialize though (or perhaps I misunderstand what the suggestion is). This is a private class though, and this whole containing class is targeted for a large refactoring as part of the cram overhaul I'm starting.

yfarjoun · 2018-09-06T22:48:18Z

src/main/java/htsjdk/samtools/BAMStartingAtIteratorFilter.java

+            return FilteringIteratorState.CONTINUE_ITERATION;
+        }
+    }
+


extra nl (from previous code)

yfarjoun · 2018-09-06T22:48:32Z

src/main/java/htsjdk/samtools/BAMStartingAtIteratorFilter.java

@@ -0,0 +1,43 @@
+package htsjdk.samtools;


why not htsjdk.samtools.filter ?

To me, this seems to have more affinity with BAMIteratorFilter and it's other subclass BAMQueryMultipleIntervalsIteratorFilter, which are all in this package. Happy to move it if you still think its better there though.

yfarjoun · 2018-09-06T22:52:53Z

src/main/java/htsjdk/samtools/CRAMFileReader.java

@@ -492,21 +494,30 @@ void enableFileSource(final SamReader reader, final boolean enabled) {
        return BAMFileSpan.merge(spanArray).toCoordinateArray();
    }

-    private class CRAMIntervalIterator extends BAMQueryMultipleIntervalsIteratorFilter
+    private abstract class CRAMIntervalIteratorBase extends BAMQueryMultipleIntervalsIteratorFilter


it would be good to note here the reason for the abstractness, not only in line 513....or better, actually have an abstract method whose implementation will resolve the issue.

Comment added. Not sure I see understand how the suggestion to add another abstract method resolves the initialize issue though. Can you give a more specific example ?

yfarjoun

Hey! work on CRAM is happening!! hurray!

pshapiro4broad · 2018-09-10T18:29:34Z

src/main/java/htsjdk/samtools/CRAMFileReader.java

                    case MATCHES_FILTER:
                        nextRec = nextRecord;
                        break;
                    case CONTINUE_ITERATION:
                        continue;
                    case STOP_ITERATION:
-                        break;
+                        nextRec = null;


Under what circumstances can you reach this line and have nextRec not be null? It's not modified outside of this method. Making it a private field would make this a little clearer.

Yes, the assignment seems to be redundant. fixed.

pshapiro4broad · 2018-09-10T18:36:30Z

src/test/java/htsjdk/samtools/CRAMFileBAIIndexTest.java

@@ -51,43 +51,43 @@
    @Test
    public void testConstructors () throws IOException {
        CRAMFileReader reader = new CRAMFileReader(cramFile, indexFile, source, ValidationStringency.SILENT);
-        CloseableIterator<SAMRecord> iterator = reader.queryAlignmentStart("chrM", 1500);
+        CloseableIterator<SAMRecord> iterator = reader.queryAlignmentStart("chrM", 1519);


Using a named constant instead of a magic number would avoid the need to modify it in many places the next time this changes. And also make it more apparent that the same value is used.

Done, including using a constant for the sequence name.

pshapiro4broad · 2018-09-10T18:36:49Z

src/test/java/htsjdk/samtools/CRAMFileBAIIndexTest.java

        Assert.assertTrue(iterator.hasNext());
        SAMRecord record = iterator.next();

        Assert.assertEquals(record.getReferenceName(), "chrM");
-        Assert.assertTrue(record.getAlignmentStart() >= 1500);
+        Assert.assertEquals(record.getAlignmentStart(),1519);


missing space after , on many lines here

pshapiro4broad · 2018-09-10T18:38:58Z

src/test/java/htsjdk/samtools/CRAMIndexQueryTest.java

+            final String queryContig,
+            final int alignmentStart,
+            final int expectedReadCount) throws IOException
+    {


open brace should be at end of previous line, not on its own line

CRAM queryAlignmentStart/queryMate fix.

59b9438

yfarjoun reviewed Sep 6, 2018

View reviewed changes

src/test/java/htsjdk/samtools/CRAMIndexQueryTest.java Show resolved Hide resolved

yfarjoun reviewed Sep 6, 2018

View reviewed changes

src/test/java/htsjdk/samtools/CRAMIndexQueryTest.java Show resolved Hide resolved

yfarjoun reviewed Sep 6, 2018

View reviewed changes

pshapiro4broad reviewed Sep 10, 2018

View reviewed changes

yfarjoun added cram Waiting for revisions This PR has received comments from reviewers and is waiting for the Author to respond labels Sep 10, 2018

cmnbroad force-pushed the cn_cram_query_mate branch 2 times, most recently from 23807f1 to ef537e8 Compare May 10, 2019 13:46

Code review comments.

ef537e8

cmnbroad added 1 - Ready Waiting for Review This PR is waiting for a reviewer to respond and removed Waiting for revisions This PR has received comments from reviewers and is waiting for the Author to respond 1 - Ready labels May 10, 2019

cmnbroad mentioned this pull request Jun 11, 2019

Read no_ref crams #1377

Closed

5 tasks

pshapiro4broad approved these changes Jun 24, 2019

View reviewed changes

cmnbroad merged commit 3a35b89 into samtools:master Jun 25, 2019

This was referenced Jun 25, 2019

queryMate method throws exception for CRAM records with the same read name #1065

Closed

Enforcing CRAM spec compliance (must have nonzero External Content ID) causes test failures #1232

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CRAM queryAlignmentStart/queryMate fix. #1164

CRAM queryAlignmentStart/queryMate fix. #1164

cmnbroad commented Jul 12, 2018

codecov-io commented Sep 6, 2018 •

edited

Loading

yfarjoun Sep 6, 2018

pshapiro4broad Sep 10, 2018 •

edited

Loading

cmnbroad May 7, 2019 •

edited

Loading

yfarjoun Sep 6, 2018

yfarjoun Sep 6, 2018

yfarjoun Sep 6, 2018

cmnbroad May 7, 2019 •

edited

Loading

yfarjoun Sep 6, 2018

cmnbroad May 7, 2019

yfarjoun left a comment

pshapiro4broad Sep 10, 2018

cmnbroad May 7, 2019

pshapiro4broad Sep 10, 2018

cmnbroad May 7, 2019

pshapiro4broad Sep 10, 2018

cmnbroad May 7, 2019

pshapiro4broad Sep 10, 2018


		/**
		* Subclasses must call this method in their constructors AFTER construction of this class is complete.

CRAM queryAlignmentStart/queryMate fix. #1164

CRAM queryAlignmentStart/queryMate fix. #1164

Conversation

cmnbroad commented Jul 12, 2018

codecov-io commented Sep 6, 2018 • edited Loading

Codecov Report

Choose a reason for hiding this comment

pshapiro4broad Sep 10, 2018 • edited Loading

Choose a reason for hiding this comment

cmnbroad May 7, 2019 • 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 May 7, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yfarjoun 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

codecov-io commented Sep 6, 2018 •

edited

Loading

pshapiro4broad Sep 10, 2018 •

edited

Loading

cmnbroad May 7, 2019 •

edited

Loading

cmnbroad May 7, 2019 •

edited

Loading