-
Notifications
You must be signed in to change notification settings - Fork 371
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
This PR skips avoids merging fingerprints when there's only one finge… #1826
This PR skips avoids merging fingerprints when there's only one finge… #1826
Conversation
This looks to be equivalent to the old code when there are > 1 fingerprints, and then handles the = 1 case manually. Are there any existing tests somewhere which cover the = 1 case to ensure nothing breaks? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just a couple of nit-picks, otherwise looks good. Thanks @yfarjoun!
final Fingerprint sampleFp = new Fingerprint(firstDetail.sample, null, by.apply(firstDetail)); | ||
entry.getValue().stream().map(Map.Entry::getValue).collect(Collectors.toSet()).forEach(sampleFp::merge); | ||
return sampleFp; | ||
Set<Fingerprint> fingerprintsSet = entry.getValue().stream().map(Map.Entry::getValue).collect(Collectors.toSet()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
final
@@ -151,21 +151,28 @@ public static Map<FingerprintIdDetails, Fingerprint> mergeFingerprintsBy( | |||
.collect(Collectors.toMap( | |||
entry -> { | |||
// merge the keys (unequal values are eliminated by merge). | |||
|
|||
List<Map.Entry<FingerprintIdDetails, Fingerprint>> entryList = entry.getValue(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
final
@yfarjoun If you still need this can you do a rebase and then 👍 to merge? |
…rprint to be merged. The merge operation performs a deep copy of the haplotype probabilities array and effectively doubles the memory requirements of the program after having loaded the fingerprints from the sources.
…takes that much memory....
180b998
to
6f8a91c
Compare
6f8a91c
to
55de0c6
Compare
…rprint to be merged. The merge operation performs a deep copy of the haplotype probabilities array and effectively doubles the memory requirements of the program after having loaded the fingerprints from the sources.
Description
Give your PR a concise yet descriptive title
Please explain the changes you made here.
Explain the motivation for making this change. What existing problem does the pull request solve?
Mention any issues fixed, addressed or otherwise related to this pull request, including issue numbers or hard links for issues in other repos.
You can delete these instructions once you have written your PR description.
Checklist (never delete this)
Never delete this, it is our record that procedure was followed. If you find that for whatever reason one of the checklist points doesn't apply to your PR, you can leave it unchecked but please add an explanation below.
Content
Review
For more detailed guidelines, see https://github.com/broadinstitute/picard/wiki/Guidelines-for-pull-requests