Repository navigation
Conversation
Reject unmapped reads inside the caller instead of filtering them out in ConsensusCallingIterator, so they are counted and written to `--rejects` rather than disappearing without trace. The parent commit dropped unmapped reads in the iterator's pre-group filter. That fixed the defect but made the reads invisible: they were never counted in `raw_reads_considered`, carried no rejection reason, and never reached the rejects output. A read that vanishes between input and output with nothing in the metrics to explain it is hard to distinguish from a bug. Move the decision into `UmiConsensusCaller.consensusReadsFromSamRecords`, the final method every caller funnels through, and reject via a new `RejectionReason.Unmapped`. All three tools are still covered by construction, and because the rejection happens on the caller the counts aggregate through `addStatistics` exactly as every other reason does, including under `--threads > 1`. The iterator's predicate returns to `mapped || (paired && mateMapped)`. Templates with no mapped read at all are still dropped there, unchanged; only the unmapped end of a half-mapped pair now flows through to be rejected explicitly. `CallMolecularConsensusReadsTest` is tightened accordingly: it now pins the exact rejects content, the new `raw_reads_rejected_for_unmapped` count, and `raw_reads_considered`, which counts the unmapped read again now that the caller sees it. Full suite: 1608 tests, 0 failures.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## nh_test-consensus-cap-half-mapped-pair #1169 +/- ##
=======================================================================
Coverage 95.95% 95.95%
=======================================================================
Files 132 132
Lines 8347 8349 +2
Branches 952 940 -12
=======================================================================
+ Hits 8009 8011 +2
Misses 338 338
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Stacked on #1168 — base is
nh_test-consensus-cap-half-mapped-pair, so review that one first. The diff here is the third commit only.Problem
#1168 stopped unmapped reads contributing to consensus reads by dropping them in
ConsensusCallingIterator's pre-group filter. That fixed the defect but made the reads invisible: they were never counted inraw_reads_considered, carried no rejection reason, and never reached--rejects.A read that disappears between input and output with nothing in the metrics to explain it is hard to tell apart from a bug. It also sits awkwardly next to every other way a read can be excluded from a consensus, all of which are counted and emitted.
Fix
Move the decision from the iterator into
UmiConsensusCaller.consensusReadsFromSamRecords— thefinalmethod every caller funnels through — and reject via a newRejectionReason.Unmapped:Structural coverage of all three tools is preserved, and because the rejection now happens on the caller, the counts aggregate through
addStatisticsexactly as every other reason does — including under--threads > 1, where each thread has its own caller viaemptyClone().The iterator's predicate returns to
mapped || (paired && mateMapped). Templates with no mapped read at all are still dropped there, unchanged; only the unmapped end of a half-mapped pair now flows through to be rejected explicitly.The reason is marked as used by all three callers (
usedByVanilla/usedByDuplex/usedByCodecall true), soraw_reads_rejected_for_unmappedis initialised to zero and appears in every stats file rather than materialising only when it fires.Behaviour
For input containing half-mapped pairs, relative to #1168:
raw_reads_consideredcounts the unmapped read again, as it did before fix(umi): exclude unmapped reads from consensus calling #1168raw_reads_rejected_for_unmappedis a new key in the stats output--rejects, tagged with therrrejection-reason tag like any other rejected recordNo change to which reads contribute to a consensus — #1168 settled that, and its tests still pin it.
Tests
CallMolecularConsensusReadsTest's two cases are tightened from a permissive check to exact assertions: the precise rejects content, the newraw_reads_rejected_for_unmappedcount, andraw_reads_considered. The permissive form was deliberate in #1168 so it would hold under either accounting choice; now that the choice is made, it pins it.Full suite: 1608 tests, 0 failures.
Note on fgumi parity
I originally raised this as an fgbio-vs-fgumi asymmetry. That framing was not quite right and is worth stating plainly: fgumi's default path also drops unmapped reads silently, in
consensus_pregroup_keep_raw, which is a plain record filter that records nothing. ItsRejectionReason::Unmappedfires only on the--allow-unmappedpath.So this change is not restoring parity — it puts fgbio ahead, and the equivalent accounting on the fgumi default path (fulcrumgenomics/fgumi#737) is now the gap. Worth doing there too, but it does not belong in that PR either.