Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
wrap both path and index #785
Conversation
jean-philippe-martin
added some commits
Dec 15, 2016
|
jean-philippe-martin |
8bb3e56
|
|||
|
|
jean-philippe-martin |
Some checks were not successful
codecov/project
— Absolute coverage decreased by -0.006% but relative coverage increased by +2.785% compared to 5a9d819
|
6693de6
|
codecov-io
commented
Jan 12, 2017
•
Current coverage is 63.869% (diff: 71.429%)@@ master #785 diff @@
==========================================
Files 523 523
Lines 31643 31640 -3
Methods 0 0
Messages 0 0
Branches 6766 6766
==========================================
- Hits 20214 20208 -6
- Misses 9293 9295 +2
- Partials 2136 2137 +1
|
jean-philippe-martin
referenced
this pull request
in broadinstitute/gatk
Jan 13, 2017
Merged
Add cloud prefetching to GATK #2331
droazen
self-assigned this
Jan 13, 2017
droazen
requested changes
Jan 13, 2017
Review complete -- minor comments only, we can merge after they're addressed. Back to @jean-philippe-martin
| @@ -82,9 +82,15 @@ | ||
| abstract public SamReader open(final File file); | ||
| public SamReader open(final Path path) { | ||
| - final SamInputResource r = SamInputResource.of(path, getPathWrapper()); | ||
| + return open(path, getPathWrapper(), Function.identity()); |
droazen
Jan 13, 2017
Contributor
It seems like we should use the wrapper returned by getPathWrapper() as the default for both the bam and its index, instead of just the bam?
jean-philippe-martin
Jan 18, 2017
Contributor
Actually since we've now moved to passing the wrappers explicitly to open, I suggest we remove setPathWrapper. The open call without wrappers will not apply any wrapper, behaving like the other methods that take optional wrapper arguments.
| + return open(path, getPathWrapper(), Function.identity()); | ||
| + } | ||
| + | ||
| + public SamReader open(final Path path, |
| + | ||
| + public SamReader open(final Path path, | ||
| + Function<SeekableByteChannel, SeekableByteChannel> pathWrapper, | ||
| + Function<SeekableByteChannel, SeekableByteChannel> indexWrapper) { |
droazen
Jan 13, 2017
•
Contributor
Is there a use case for having different wrappers for the bam vs. the index? If not, should we just have a single wrapper argument to this constructor that is applied to both (assuming it's not problematic to share wrappers)?
jean-philippe-martin
Jan 18, 2017
Contributor
It's not problematic to share wrappers (they have no state). Those files are accessed differently, so it would be a great coincidence if the best caching&prefetching for one were also the best for the other.
| final Path indexMaybe = SamFiles.findIndex(path); | ||
| - if (indexMaybe != null) r.index(indexMaybe); | ||
| + if (indexMaybe != null) r.index(indexMaybe, indexWrapper); | ||
| return open(r); | ||
| } |
droazen
Jan 13, 2017
•
Contributor
Is it possible to add a simple unit test for this method that verifies that the provided wrappers are applied (perhaps using a dummy wrapper of some kind)?
jean-philippe-martin
Jan 19, 2017
Contributor
One way would be to copy the xor wrapper from GATK and check that we can read an XOR'd index.
jean-philippe-martin
Jan 19, 2017
Contributor
Actually that wouldn't work because the factory only uses the wrappers for things that are not files. Instead I'm doing something simpler and directly calling "asUnbufferedSeekableStream" and checking that the wrapper was accessed.
droazen
assigned jean-philippe-martin and unassigned droazen
Jan 13, 2017
|
|
jean-philippe-martin |
Some checks were not successful
codecov/project
— Absolute coverage decreased by -0.019% but relative coverage increased by +7.547% compared to 5a9d819
|
496dcbf
|
|
@droazen the code is ready! (Mysteriously, github isn't letting me assign the bug back to you. Perhaps something needs to be done to grant me bug-assigner permissions?) |
droazen
assigned droazen and unassigned jean-philippe-martin
Jan 20, 2017
| +import java.nio.file.Paths; | ||
| +import java.util.HashMap; | ||
| +import java.util.function.Function; | ||
| +import junit.framework.Assert; |
|
@jean-philippe-martin Looks good, just one import in the tests needs fixing, then I'll hit merge! |
droazen
assigned jean-philippe-martin and unassigned droazen
Jan 20, 2017
|
|
jean-philippe-martin |
Some checks were not successful
codecov/project
— Absolute coverage decreased by -0.013% but relative coverage increased by +7.547% compared to 5a9d819
|
031cee9
|
jean-philippe-martin commentedJan 11, 2017
•
edited
Description
Extend optional wrapping to the index.
Checklist