Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adds a buffer to the index stream #1348

Merged
merged 2 commits into from
Aug 6, 2019

Conversation

yfarjoun
Copy link
Contributor

This makes reading the index over a network possible, which in turn, fixes #1175.


Checklist (never delete this)

Content

  • Added or modified tests to cover changes and any new functionality
  • Edited the README / documentation (if applicable)
  • All tests passing on Travis

Review

  • Final thumbs-up from reviewer
  • Rebase, squash and reword as applicable

For more detailed guidelines, see https://github.com/broadinstitute/picard/wiki/Guidelines-for-pull-requests

…eading the index over a network possible, fixes #1175
@yfarjoun yfarjoun requested a review from lbergelson June 20, 2019 15:48
@coveralls
Copy link

coveralls commented Jun 20, 2019

Coverage Status

Coverage increased (+0.2%) to 79.962% when pulling def9d41 on yf_add_prefetching_to_crsscheck_fingerprints into ce50809 on master.

@yfarjoun
Copy link
Contributor Author

@lbergelson can you take a look?

Copy link
Member

@lbergelson lbergelson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two comments, do as you like and then 👍

@@ -448,6 +484,7 @@ public IntervalList getLociToGenotype(final Collection<Fingerprint> fingerprints
// Now go through the data at each locus and figure stuff out!
for (final SamLocusIterator.LocusInfo info : iterator) {

log.debug("At locus " + info.toString());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will do a pointless string combine at each position which could be kind of expensive if you don't want this output. If there's a way check if we're in debug mode first than I would test that before doing the debug command.

@@ -402,7 +438,7 @@ public IntervalList getLociToGenotype(final Collection<Fingerprint> fingerprints
final SamReader in = SamReaderFactory.makeDefault()
.enable(SamReaderFactory.Option.CACHE_FILE_BASED_INDEXES)
.referenceSequence(referenceFasta)
.open(samFile);
.open(samFile, null, seekableChannelFunction);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would add a comment here describing why this is needed and pointing at the htsjdk issue number ( is there one... there should be.)

@yfarjoun yfarjoun merged commit 2738f8e into master Aug 6, 2019
@yfarjoun yfarjoun deleted the yf_add_prefetching_to_crsscheck_fingerprints branch November 3, 2019 13:52
@yfarjoun yfarjoun restored the yf_add_prefetching_to_crsscheck_fingerprints branch August 11, 2020 16:23
@yfarjoun yfarjoun deleted the yf_add_prefetching_to_crsscheck_fingerprints branch December 22, 2020 21:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CrosscheckFingerprints NIO streaming of BAMs hangs tool indefinitely
3 participants