-
Notifications
You must be signed in to change notification settings - Fork 594
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
Fix edge case where dangling end merging creates cycles #5960
Conversation
Thanks @davidbenjamin! Please let me know when the merge is complete and we will rerun gnomAD analysis. |
Codecov Report
@@ Coverage Diff @@
## master #5960 +/- ##
==============================================
+ Coverage 86.823% 86.84% +0.017%
- Complexity 32360 32429 +69
==============================================
Files 1993 1993
Lines 149509 149725 +216
Branches 16524 16598 +74
==============================================
+ Hits 129808 130021 +213
+ Misses 13676 13673 -3
- Partials 6025 6031 +6
|
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.
One tiny question, but LGTM. Also, the codecov looks weird, not sure if that's related to this PR or not?
return getAssemblyResult(refHaplotype, kmerSize, rtgraph, aligner); | ||
final AssemblyResult result = getAssemblyResult(refHaplotype, kmerSize, rtgraph, aligner); | ||
// check whether recovering dangling ends created cycles | ||
if (recoverAllDanglingBranches && rtgraph.hasCycles()) { |
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.
It doesn't actually matter if recoverAllDanglingBranches
right? You want to return null if there are cycles regardless?
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.
You're right but I'm being conservative about runtime here. This is all going to be overhauled soon anyway.
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.
And don't worry about the code coverage. It's effectively a random number generator.
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.
Sounds good, merge away!
@meganshand This fixes the Sarah's recent bug, and just makes sense in general.
I am not adding a regression test because our work on linked de Bruijn graphs is going to moot stuff like this soon.