Conversation
cmnbroad
force-pushed
the
cn_ref_bundles
branch
from
11db598 to
94875b9
Compare
cmnbroad
force-pushed
the
cn_ref_bundles
branch
from
94875b9 to
b0a5f1c
Compare
cmnbroad
force-pushed
the
cn_ref_bundles
branch
from
b0a5f1c to
ae17645
Compare
lbergelson
left a comment
lbergelson
left a comment
There was a problem hiding this comment.
@cmnbroad This looks good to me. I have a few style questions and typos, feel free to do what you want with those though.
| } | ||
|
|
||
| /** | ||
| * Create q reference bundle given only a fasta path, including an index and a dictionary |
| } | ||
|
|
||
| /** | ||
| * Create q reference bundle given only a fasta path, including an index and a dictionary |
There was a problem hiding this comment.
| * Create q reference bundle given only a fasta path, including an index and a dictionary | |
| * Create a reference bundle given only a fasta path, including an index and a dictionary |
|
|
||
| /** | ||
| * Constructs a FastaSequenceFile that reads from the specified fasta and dictionary file. Makes no | ||
| * assumptions that the fata and dict file are in the same directory. |
There was a problem hiding this comment.
| * assumptions that the fata and dict file are in the same directory. | |
| * assumptions that the fasta and dict file are in the same directory. |
| * Open the given indexed fasta sequence file. Throw an exception if the file cannot be opened. | ||
| * | ||
| * @param path The file to open. | ||
| * @param dictPath the dictionar path (may be null) |
There was a problem hiding this comment.
| * @param dictPath the dictionar path (may be null) | |
| * @param dictPath the dictionary path (may be null) |
| if (IOUtil.isBlockCompressed(path.toPath(), true)) { | ||
| throw new SAMException("Indexed block-compressed FASTA file cannot be handled: " + path); | ||
| } | ||
| this.channel = Files.newByteChannel(path.toPath()); |
There was a problem hiding this comment.
I wonder if we should add getChannel methods to IOPath. We have getStream ones.
| if (!truncateNamesAtWhitespace) { | ||
| throw new RuntimeException("preferIndexed option requires truncateNamesAtWhitespace"); | ||
| } | ||
| final Optional<BundleResource> indexPathResource = referenceBundle.get(BundleResourceType.CT_REFERENCE_INDEX); |
There was a problem hiding this comment.
These chains of optional if-present-get are kind of gross. What do you think of flatmap chains instead? I think they're a little more readable, at least once you know what you're looking at.
IOPath indexPath = referenceBundle.get(BundleResourceType.CT_REFERENCE_INDEX)
.flatMap(BundleResource::getIOPath)
.orElse(null);
if (preferIndexed && indexPath != null !Files.exists(indexPath.toPath())) {
throw new RuntimeException(String.format("FASTA index file %s does not exist", indexPath));
}
| final IOPath remoteIndex = new HtsPath( | ||
| Files.copy( | ||
| indexFile.toPath(), | ||
| nioFastaDir.resolve(indexFile.getBaseName().get() + indexFile.getExtension().get())).toUri().toString()); |
There was a problem hiding this comment.
I think you meant indexDir here?
There was a problem hiding this comment.
These copy blocks seem like they could be extracted into a function, it might help avoid this copy paste sorts of errors.
lbergelson
left a comment
lbergelson
left a comment
There was a problem hiding this comment.
@cmnbroad I made the changes I requested since I don't think you'll get to this anytime soon.
2 participants
The main feature implemented here is that a bundle can be used in place of a reference, without executing any code paths that assume that the individual resources are located in the same parent directory (there are a LOT of such code paths, since most of the existing code base makes these assumptions). There are a number of tests added that exist purely to catch any regressions where such assumptions are reintroduced, at lease in the code paths where bundles are enabled.