-
-
Notifications
You must be signed in to change notification settings - Fork 69
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
Ensure --min-reads
is a hard filter in CallDuplexConsensusReads
#1010
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1010 +/- ##
==========================================
+ Coverage 95.62% 95.64% +0.01%
==========================================
Files 126 126
Lines 7388 7395 +7
Branches 510 498 -12
==========================================
+ Hits 7065 7073 +8
+ Misses 323 322 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
if (consensus.forall(duplexHasMinimumNumberOfReads)) { | ||
consensus | ||
} else { | ||
rejectRecords(groups.flatten, FilterMinReads) |
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.
We reject the original reads in the group, and not the consensus.
src/test/scala/com/fulcrumgenomics/umi/DuplexConsensusCallerTest.scala
Outdated
Show resolved
Hide resolved
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.
I took a brief look in VanillaUmiConsensusCaller
and it doesn't seem to share this bug, right? I.e. it already re-checks to see if it has enough reads after filtering to the most common alignment.
Feel free to merge after fixing up minor suggestions.
src/main/scala/com/fulcrumgenomics/umi/DuplexConsensusCaller.scala
Outdated
Show resolved
Hide resolved
src/main/scala/com/fulcrumgenomics/umi/DuplexConsensusCaller.scala
Outdated
Show resolved
Hide resolved
Yes, you are correct! Documented the reasoning in the original issue too: #1009 (comment) We should be all set after this PR is merged. |
Closes #1009