Fixes and tests for CRAM index queries. #552

cmnbroad · 2016-04-04T14:32:42Z

Includes fixes for #481, #550, #551, and #562;

droazen · 2016-04-04T15:24:10Z

@vadimzalunin Can you review? We were somewhat surprised to learn that cram queries are broken in so many cases..

droazen · 2016-04-07T19:20:57Z

src/java/htsjdk/samtools/CRAMFileReader.java

+                if (alignEnd == 0) {
+                    // handle placed but "unmapped" since they can pass the overlap test
+                    return (alignStart >= interval.start) &&
+                            (interval.end <= 0 || alignStart <= interval.end);


Handle case where alignStart < interval.start && alignEnd == 0 in a manner consistent with the BAM-querying code.

(BAMQueryMultipleIntervalsIteratorFilter.compareIntervalToRecord() suggests that unmapped reads with a start position are assigned an end position equal to start for BAM queries)

cmnbroad · 2016-04-09T19:51:19Z

I've added a second commit to this PR with fixes for the comments here, as well as a proposed fix for #551. The second commit reduces the two CRAMFileReader interval iterator implementations (IntervalIterator and MultipleIntervalIterator) into a single implementation that uses (a temporarily cloned copy of the) interval matching code used by the BAM iterators iterators to do interval matching. If we get agreement that this is the right strategy, I'll refactor the cloned code so it can be shared between the implementations.

vadimzalunin · 2016-04-14T13:09:03Z

@cmnbroad the approach looks good. A shared iterator impl would be ideal.

cmnbroad · 2016-04-26T18:58:16Z

This now includes fixes for #481, #550, #551, and #562; uses a multiple interval filter iterator implementation that is shared with BAMFileReader; and adds a bunch of new CRAM tests.

cmnbroad · 2016-04-26T18:59:03Z

@vadimzalunin Please review. thx.

cmnbroad · 2016-04-29T15:13:29Z

src/java/htsjdk/samtools/CRAMFileReader.java

-            }
-        }
-        throw new SAMException("Failed to query alignment start: " + sequence + " at " + start);
+        return new CRAMIntervalIterator(new QueryInterval[]{new QueryInterval(referenceIndex, start, -1)}, true);
    }

    CloseableIterator<SAMRecord> query(final int referenceIndex,
                                       final int start, final int end, final boolean overlap) throws IOException {


Note to self: Delete this method. I meant to remove it as its no longer used internally and should just be deleted (its not part of the public query interface, and is confusing since it reverses the polarity of contained/overlap compared to the other methods).

👍 on deletion if possible

droazen · 2016-05-06T18:50:28Z

@vadimzalunin Do you have time to do a final review on this? This is blocking us from safely using CRAM as input to the GATK, since our queries on CRAMs are currently not returning the correct results. Thanks!

droazen · 2016-05-19T19:45:14Z

src/java/htsjdk/samtools/BAMQueryMultipleIntervalsIteratorFilter.java

+/**
+ * Filters out records that do not match any of the given intervals and query type.
+ */
+public class BAMQueryMultipleIntervalsIteratorFilter implements BAMIteratorFilter {


Since you've made this formerly private class public, can you add direct unit tests for it?

droazen · 2016-05-19T20:38:41Z

src/tests/java/htsjdk/samtools/CRAMIndexQueryTest.java

+                    new String[]{"a", "b", "c", "d", "e"}},
+            {cramQueryWithBAI, cramQueryReference,
+                    new QueryInterval[]{new QueryInterval(0, 100010, 100010), new QueryInterval(0, 100011, 100011)},
+                    new String[]{"a", "b", "c", "d", "e"}},


Recommend having more than two test cases here. Should have tests involving multiple contigs, as well as more than two intervals, and also cover cases like no reads overlap any intervals, first read is before the first interval, first interval is before the first read, etc. If your existing test file doesn't have enough reads/contigs, can use something like src/test/resources/org/broadinstitute/hellbender/engine/reads_data_source_test1.bam from the GATK, which uses a tiny reference.

Can also try porting the test cases from ReadsDataSourceUnitTest here, which use the reads_data_source_test1.bam file mentioned above.

Added quite a few more tests for both overlapped and contained.

droazen · 2016-05-19T20:48:53Z

First-pass review complete, back to @cmnbroad

cmnbroad · 2016-05-27T18:56:43Z

Didn't squash or rebase yet but otherwise I think this covers it.

droazen · 2016-06-09T20:32:00Z

src/java/htsjdk/samtools/CRAMFileReader.java

+                } catch (final IOException e) {
+                    throw new RuntimeEOFException(e);
+                }
+                next(); // advance to the first record that matches the filter criteria


Have you confirmed that this iterator works with an empty cram file?

Added a couple of tests to validate this.

droazen · 2016-06-09T21:16:51Z

Review complete, back to @cmnbroad. Merge after addressing remaining comments and rebasing.

coveralls · 2016-06-10T20:09:06Z

Coverage increased (+0.07%) to 68.141% when pulling 12b0345 on cmnbroad:cn_cram_query_fix into 6237f7d on samtools:master.

droazen assigned vadimzalunin Apr 4, 2016

cmnbroad force-pushed the cn_cram_query_fix branch from 74e0dda to 1a5b896 Compare April 4, 2016 16:11

droazen reviewed Apr 7, 2016
View reviewed changes

cmnbroad changed the title ~~Fixes and tests for CRAM single-interval index queries.~~ Fixes and tests for CRAM index queries. Apr 9, 2016

cmnbroad mentioned this pull request Apr 9, 2016

CRAM multi-interval queries return multiple instances of the same record #551

Closed

cmnbroad force-pushed the cn_cram_query_fix branch from dfd2d39 to 4fec505 Compare April 26, 2016 18:37

cmnbroad assigned cmnbroad and vadimzalunin and unassigned vadimzalunin and cmnbroad Apr 26, 2016

This was referenced Apr 26, 2016

Cram query using .crai returns incorrect results broadinstitute/gatk#1673

Closed

Extend CRAM test case matrix #592

Closed

cmnbroad force-pushed the cn_cram_query_fix branch from 4fec505 to 01b87ee Compare April 27, 2016 18:44

cmnbroad mentioned this pull request Apr 28, 2016

extremely slow CRAM processing of intervals broadinstitute/gatk#1787

Closed

cmnbroad reviewed Apr 29, 2016
View reviewed changes

cmnbroad mentioned this pull request May 6, 2016

Make interface changes to allow CRAM to work with IGV #597

Closed

cmnbroad added the version label May 11, 2016

droazen reviewed May 19, 2016
View reviewed changes

droazen assigned cmnbroad and unassigned vadimzalunin May 19, 2016

cmnbroad force-pushed the cn_cram_query_fix branch 3 times, most recently from 5b14052 to 9db6fdc Compare May 27, 2016 18:53

cmnbroad assigned cmnbroad and droazen and unassigned cmnbroad May 27, 2016

yfarjoun mentioned this pull request May 29, 2016

Review "Party" #548

Closed

yfarjoun added the Review-party candidate label May 29, 2016

cmnbroad mentioned this pull request May 31, 2016

Implement CRAI writing. #538

Merged

droazen reviewed Jun 9, 2016
View reviewed changes

droazen assigned cmnbroad and unassigned droazen Jun 9, 2016

cmnbroad force-pushed the cn_cram_query_fix branch from 9db6fdc to 7c4f032 Compare June 10, 2016 18:25

Fixes and tests for CRAM index queries.

12b0345

cmnbroad force-pushed the cn_cram_query_fix branch from 7c4f032 to 12b0345 Compare June 10, 2016 19:58

cmnbroad merged commit f5ff9e4 into samtools:master Jun 12, 2016

This was referenced Jun 13, 2016

CRAM files do not support zero for start/end coordinates when doing interval queries #481

Closed

CRAM queryUnmapped returns incorrect values with recordPerSlice=2000 #562

Closed

cmnbroad deleted the cn_cram_query_fix branch March 7, 2017 01:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes and tests for CRAM index queries. #552

Fixes and tests for CRAM index queries. #552

cmnbroad commented Apr 4, 2016 •

edited

droazen commented Apr 4, 2016

droazen Apr 7, 2016

droazen Apr 7, 2016

cmnbroad commented Apr 9, 2016

vadimzalunin commented Apr 14, 2016

cmnbroad commented Apr 26, 2016

cmnbroad commented Apr 26, 2016

cmnbroad Apr 29, 2016 •

edited

droazen May 19, 2016

cmnbroad May 27, 2016

droazen commented May 6, 2016

droazen May 19, 2016

cmnbroad May 27, 2016

droazen May 19, 2016 •

edited

droazen May 19, 2016

cmnbroad May 27, 2016

droazen commented May 19, 2016

cmnbroad commented May 27, 2016

droazen Jun 9, 2016

cmnbroad Jun 10, 2016

droazen commented Jun 9, 2016 •

edited

coveralls commented Jun 10, 2016 •

edited

Fixes and tests for CRAM index queries. #552

Fixes and tests for CRAM index queries. #552

Conversation

cmnbroad commented Apr 4, 2016 • edited

droazen commented Apr 4, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cmnbroad commented Apr 9, 2016

vadimzalunin commented Apr 14, 2016

cmnbroad commented Apr 26, 2016

cmnbroad commented Apr 26, 2016

cmnbroad Apr 29, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

droazen commented May 6, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

droazen May 19, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

droazen commented May 19, 2016

cmnbroad commented May 27, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

droazen commented Jun 9, 2016 • edited

coveralls commented Jun 10, 2016 • edited

cmnbroad commented Apr 4, 2016 •

edited

cmnbroad Apr 29, 2016 •

edited

droazen May 19, 2016 •

edited

droazen commented Jun 9, 2016 •

edited

coveralls commented Jun 10, 2016 •

edited