fix(codec)!: downsample after filtering to the most common alignment - #731
Conversation
|
Note Reviews pausedUse the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughPre-filter capping could reject families that fgbio would consensus. The pipeline now filters alignments, validates minimum reads, and caps each strand independently. Tests and ChangesCODEC consensus downsampling
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant consensus_reads_raw
participant AlignmentFilter
participant ReadCap
participant ConsensusCaller
consensus_reads_raw->>AlignmentFilter: Filter each strand to its common alignment
AlignmentFilter-->>consensus_reads_raw: Return filtered reads
consensus_reads_raw->>ReadCap: Apply validated per-strand max-reads cap
ReadCap-->>ConsensusCaller: Return retained reads
ConsensusCaller-->>consensus_reads_raw: Generate strand consensus
Possibly related PRs
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
Comment |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #731 +/- ##
==========================================
- Coverage 94.12% 94.11% -0.01%
==========================================
Files 181 181
Lines 109415 109503 +88
==========================================
+ Hits 102987 103062 +75
- Misses 6428 6441 +13 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
@coderabbitai pause |
✅ Action performedReviews paused. |
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/fgumi-consensus/src/codec_caller.rs`:
- Around line 4671-4703: FLAG MISSING COVERAGE for divergent per-strand
retention. Extend the consensus_reads_from_sam_records test fixture so R1 and R2
filtering retains different template sets, rather than sharing each template’s
CIGAR, then assert that each strand’s cap is computed from its own filtered
reads instead of capping pairs jointly.
- Around line 740-742: Validate max_reads_per_strand at the options boundary so
zero is rejected and cannot reach the capping logic in the surrounding
codec-caller flow. Also guard the existing max_reads_per_strand branch before
both cap_infos_to_lowest_ranking calls, ensuring programmatic callers with zero
are rejected rather than emptying r1_infos and r2_infos.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 81fcdb86-07b0-48c4-9af5-7dd79da13b43
⛔ Files ignored due to path filters (1)
CHANGELOG.mdis excluded by!**/CHANGELOG.md
📒 Files selected for processing (2)
crates/fgumi-consensus/src/codec_caller.rssrc/lib/commands/codec.rs
`codec --max-reads` applied its cap before `filter_to_most_common_alignment_raw`, while fgbio filters each strand first, checks `--min-reads` on the filtered lists, and only then caps (inside `ssCaller.consensusCall`). Capping first samples across a family that still mixes alignment patterns, so the majority pattern among the sampled reads can differ from the majority in the full family. The alignment filter then cuts the family a second time and can push it below `--min-reads`, rejecting a family fgbio consenses. The cap is now applied after the filter and the read-count check, and independently per strand, matching fgbio. Nothing downstream pairs the two strand lists by index, so dropping the shared index vector is safe: phase 4 takes the longest of each list independently and phase 5 builds a separate single-strand consensus from each. On a CODEC library this recovers 397 consensus records at `--max-reads 2` (103,144 -> 103,541) and 61 at `--max-reads 3` (105,478 -> 105,539). Runs without `--max-reads` are unaffected. Because each strand is now capped on its own ranks, a template can be retained on one end and dropped on the other when only one end survives the alignment filter; the help text and CHANGELOG no longer claim otherwise. Closes #730
dceccc5 to
75dc637
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
Closes #730.
codec --max-readsapplied its cap beforefilter_to_most_common_alignment_raw, while fgbio filters each strand first, checks--min-readson the filtered lists, and only then caps — insidessCaller.consensusCall, which is constructed withmaxReadsPerStrand(CodecConsensusCaller.scala:115, 196-198, 234-235).Capping first samples across a family that still mixes alignment patterns, so the majority pattern among the sampled reads can differ from the majority in the full family. The alignment filter then cuts the family a second time and can push it below
--min-reads, rejecting a family fgbio consenses. See #730 for the worked example.The cap now runs after the filter and the read-count check, and independently per strand.
Dropping the shared index vector is safe
The old code computed one index vector from R1's ranks and applied it to both
r1_infosandr2_infos, which kept the two lists index-aligned. Nothing downstream depends on that:filter_to_most_common_alignment_rawalready ran independently per strand and routinely returns different lengths, phase 4 takes the longest of each list independently, and phase 5 builds a separate single-strand consensus from each. So the lists were already free to diverge before this change.Impact
Measured on
CODEC.group.adjacency.fgbio.bam:--max-reads 2--max-reads 3Runs that do not set
--max-readsare unaffected.Behaviour note
Because each strand is now capped on its own ranks, a template can be retained on one end and dropped on the other when only one end survives the alignment filter. That matches fgbio. The
codec --max-readshelp text and the CHANGELOG entry added in #727 claimed both ends are always retained together, which is no longer true — both are corrected here.Testing
codec_cap_runs_after_the_alignment_filterbuilds the worked example from #730 — ten templates, six carrying the majority alignment and four a minority one, with the three lowest-ranking names deliberately split across both groups — and asserts a consensus is produced. Verified failing against the previous ordering (count == 0, the family rejected asInsufficientReads) and passing after.cargo ci-fmt,ci-lintandci-testare clean: 7195 passed, 30 skipped.Stacked on #727
Based on
725/nhomer/fix-deterministic-consensus-downsampling, since it rewrites the same block. Merge #727 first; this PR retargets tomainonce that lands.Risk:
codecconsensus output changes, pinned by fgbio-compatible alignment filtering and reproducible hash-based downsampling;unsafechanges: none, and theCLAUDE.mdallowlist is unchanged; memory bounds, queue capacity, and thread/backpressure policy changes: none.Fix: Filter each strand to its majority alignment, validate
--min-reads, then downsample independently with--max-reads.--max-reads 2and 61 with--max-reads 3onCODEC.group.adjacency.fgbio.bam.--max-readsremain unchanged.