Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
Path index #762
Conversation
jean-philippe-martin
added some commits
Nov 29, 2016
|
cc: @droazen @lbergelson |
droazen
self-assigned this
Nov 29, 2016
|
Tests failed -- re-running to try to determine if the failure is real. |
coveralls
commented
Nov 29, 2016
•
droazen
requested changes
Nov 29, 2016
First-pass review complete -- a few comments/questions to address.
| @@ -311,7 +311,24 @@ public SamReader open(final SamInputResource resource) { | ||
| if (SamStreams.isBAMFile(bufferedStream)) { |
droazen
Nov 29, 2016
•
Contributor
Is it ok to leave the indexMaybe.asFile() call intact here (even if the index might be in a bucket)? If yes, add a comment explaining why.
jean-philippe-martin
Nov 30, 2016
Contributor
There is no such call on this line so I'm guessing you mean the call just before? Yes, calling toFile on a bucket has no harmful effect, it just returns null. We use other ways to get to the index inside this block, except for the case where there's no index at all (this matches the previous behavior: we pass null and the code deals with the fact there's no index). I've added a comment to this effect.
| + // let's make sure the user can seek in this file. | ||
| + // need to return to the beginning because it's the same stream we used earlier | ||
| + // and read a bit from. | ||
| + sourceSeekable.seek(0); |
droazen
Nov 29, 2016
•
Contributor
Can you explain why we don't have to seek to the beginning in the other places where bufferedStream is used (assuming bufferedStream and sourceSeekable are the same underlying stream)?
jean-philippe-martin
Nov 30, 2016
•
Contributor
I think that's because the other places were written before this patch and so they expect the non-seekable stream to start from wherever isBAMFile leaves it. You can see in the previous version of the code how the stream is passed to this variant of the constructor without rewinding. The seekable code path, instead, expects the stream to start from the beginning.
I've added comments to this effect.
| + primitiveSamReader = new BAMFileReader( | ||
| + sourceSeekable, indexSeekable, false, asynchronousIO, validationStringency, this.samRecordFactory); | ||
| + } else { | ||
| + primitiveSamReader = new BAMFileReader( |
droazen
Nov 29, 2016
Contributor
This else clause is the same as the if (indexFile != null) clause above -- is it possible to simplify this nested if/else to combine them into a single case?
| @@ -138,6 +138,31 @@ | ||
| mStream.setInputFileName(file.getAbsolutePath()); | ||
| } | ||
| + | ||
| + | ||
| + private BAMFileReader(final File file, |
| + mStream.setInputFileName(file.getAbsolutePath()); | ||
| + } | ||
| + | ||
| + static BAMFileReader fromFileAndSeekable(final File file, |
| @@ -187,7 +212,7 @@ private BAMFileReader(final BlockCompressedInputStream compressedInputStream, | ||
| final SAMRecordFactory factory) | ||
| throws IOException { | ||
| mIndexStream = indexStream; | ||
| - mIsSeekable = true; | ||
| + mIsSeekable = null != indexStream; |
droazen
Nov 29, 2016
•
Contributor
I'm concerned that with this change the treatment of mIsSeekable is now inconsistent across constructors in this class. Eg., the constructor that takes a BlockCompressedInputStream and a File for the index sets mIsSeekable to true unconditionally, regardless of whether the index file is null. We should probably attempt to impose consistency across constructors here rather than leave the class in an inconsistent state.
droazen
Nov 29, 2016
Contributor
Actually, I'm not even sure that this change is necessary, given that the implementation of BAMFileReader.hasIndex() looks like this:
public boolean hasIndex() {
return mIsSeekable && ((mIndexFile != null) || (mIndexStream != null));
}
jean-philippe-martin
Nov 30, 2016
Contributor
I think you're right and I can leave it to "true". I ran SamReaderFactoryTest and all 84 tests pass.
|
Tests passed on the second run -- I assume the initial failure was a temporary ftp connectivity issue. |
|
@droazen this should address all your comments. I ran the test suite and all 23992 tests report passing. |
|
Travis reports 2 failures, both with "Could not connect to ftp.broadinstitute.org". Sounds like the same transient we've had issues with before. |
coveralls
commented
Nov 30, 2016
|
@droazen OK for me to merge? |
jean-philippe-martin commentedNov 29, 2016
Description
Fix a bug that was preventing inputs from being specified as Paths when they are not also Files
(issue #749).
Checklist
(I wrote one, it passed, but then I had to take it out to avoid adding a dependency to htsjdk)
except for AbstractFeatureReaderTest, testFTPOpenInputStream, sourceLikeBam, and SeekableFTPStreamTest.setUp showing a (presumably normal): Could not connect to ftp.broadinstitute.org, for input source: ftp://ftp.broadinstitute.org/distribution/igv/TEST/cpgIslands with spaces.hg18.bed