Implement ReadsHtsgetData source, refactor HtsgetReader #6662
Conversation
846b5f7
to
ba2dd80
Compare
|
I'm reviewing this right now. I'm sorry it's late. |
There was a problem hiding this comment.
So this looks pretty good to start. I think there are some issues around many intervals being started in parallel and we may want to revisit that like we talked about on the call. Adding additional checks to ensure we're in coordinate order might make things simpler.
As far as I can tell the reader initialization is in parallel, which is good, but the actual reading will be in series because the merging iterator will poll one at a time. We might want to look at that later. We might also want to look into aggressively prefetching reads.
It obviously needs tests.
src/main/java/org/broadinstitute/hellbender/engine/GATKTool.java
Outdated
Show resolved
Hide resolved
src/main/java/org/broadinstitute/hellbender/engine/ReadsHtsgetDataSource.java
Outdated
Show resolved
Hide resolved
src/main/java/org/broadinstitute/hellbender/engine/ReadsHtsgetDataSource.java
Outdated
Show resolved
Hide resolved
src/main/java/org/broadinstitute/hellbender/engine/ReadsHtsgetDataSource.java
Outdated
Show resolved
Hide resolved
src/main/java/org/broadinstitute/hellbender/engine/ReadsHtsgetDataSource.java
Outdated
Show resolved
Hide resolved
src/main/java/org/broadinstitute/hellbender/transformers/SamRecordTransformer.java
Outdated
Show resolved
Hide resolved
src/main/java/org/broadinstitute/hellbender/utils/iterators/SamRecordTransformingIterator.java
Outdated
Show resolved
Hide resolved
| return new SAMRecordToReadIterator(samRecordIterator); | ||
| } | ||
|
|
||
| private Map<SamReader, CloseableIterator<SAMRecord>> getReaders(final boolean traverseUnmapped) { |
There was a problem hiding this comment.
traverseUnmapped is confusingly named here. The way I read it initially was that travereUnmapped mean "also include unmapped reads". I.e. traverseUnmapped == false implies NO unmapped unplaced reads and true means get all the reads including unplaced unmapped.
The flag actually seems to behave as "onlyIncludeUnplacedUnmappedReads". I think it should be renamed and a comment would be useful.
There was a problem hiding this comment.
Yeah this behaviour is confusing, but I'm trying to follow what ReadsPathDataSource does which is more complicated.
If there are intervals and traverseUnmapped == true it returns the intervals and the unplaced unmapped reads
If there are no intervals and traverseUnmapped == true it returns only the unplaced unmapped reads.
src/main/java/org/broadinstitute/hellbender/engine/ReadsHtsgetDataSource.java
Outdated
Show resolved
Hide resolved
|
We should be able to add tests quite easily once we can get the reference server into gatk, so merging this will have to wait until then. |
src/main/java/org/broadinstitute/hellbender/engine/ReadsHtsgetDataSource.java
Outdated
Show resolved
Hide resolved
| @@ -0,0 +1,10 @@ | |||
| WORKING_DIR=/home/travis/build/broadinstitute | |||
There was a problem hiding this comment.
Please add a comment to this script explaining what it does.
There was a problem hiding this comment.
Renamed file and added a README to the script folder
src/main/java/org/broadinstitute/hellbender/engine/ReadsHtsgetDataSource.java
Outdated
Show resolved
Hide resolved
src/main/java/org/broadinstitute/hellbender/engine/ReadsHtsgetDataSource.java
Outdated
Show resolved
Hide resolved
src/main/java/org/broadinstitute/hellbender/tools/htsgetreader/HtsgetRequest.java
Outdated
Show resolved
Hide resolved
src/test/java/org/broadinstitute/hellbender/engine/ReadsHtsgetDataSourceUnitTest.java
Outdated
Show resolved
Hide resolved
src/test/resources/org/broadinstitute/hellbender/engine/htsget_config.json
Outdated
Show resolved
Hide resolved
src/test/java/org/broadinstitute/hellbender/engine/ReadsHtsgetDataSourceUnitTest.java
Outdated
Show resolved
Hide resolved
* Switch to using htsjdk versions of htsget primitives * Update unit tests
Allow tools to read from htsget data sources.
@lbergelson