Repository navigation
fix(dedup)!: report per-reason filtered template counts - #740
Merged
Merged
Conversation
Introduce TemplateFilterReason and TemplateFilterCounts: one variant per rejection site in the pre-grouping template filter, and a collector that records every decision in both template and primary-read units. This replaces the practice of reusing group's fgbio output schema as the measurement primitive, which capped the reason vocabulary at fgbio's four discard columns. Named template_filter rather than filter because fgumi filter is a real command with its own metrics, and every module in this crate is named for the command it measures.
The struct-level test pins UmiGroupingMetrics' serialization; this pins what the command actually writes, so an internal refactor of the filter accounting cannot change the file fgbio has to be able to read.
Replace FilterMetrics with TemplateFilterCounts in both filters. FilterMetrics was group's fgbio output schema used as a measurement primitive: four discard buckets named after fgbio's columns, which dedup borrowed while counting a different unit into them (its `accepted_templates` field held a primary-read count in group and a template count in dedup). group's .grouping_metrics.txt is unchanged -- verified byte-identical across three filter regimes -- because UmiGroupingMetrics::set_filter_counts projects the nine reasons back onto fgbio's four columns in primary-record units. dedup's metrics file is also unchanged here; wiring the new counts into its output is a separate change. Both filters are converted together because deleting FilterMetrics breaks dedup, and every commit must build. The existing filter tests now assert the specific reason rather than the bucket, which immediately caught a mislabelled case: the dedup truncated-aux-record test was reported as a poor-alignment rejection when the record actually clears the length check and is rejected for a missing UMI.
Distinguish the per-worker accumulator from the serializable metrics schema, which moves to fgumi-metrics as DeduplicationMetrics.
Move dedup's metrics file schema into the fgumi-metrics crate and give it a column per template filter reason, plus passthrough_templates for the --include-unmapped templates that bypass the filter and so appeared in no tally at all. Living in this crate also means the metrics reference page is generated by cargo xtask like every other metric type, rather than dedup being the one schema with no documentation. Nothing writes these columns yet; wiring the counts into the output file is the next change.
|
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 (4)
WalkthroughThe PR adds shared template filtering and reason-based counters, exposes reusable metrics types, updates ChangesShared filtering and metrics
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant DedupCommand
participant filter_template
participant DedupCounts
participant DeduplicationMetrics
participant MetricsFile
DedupCommand->>filter_template: filter templates
filter_template->>DedupCounts: record acceptance or rejection reason
DedupCommand->>DedupCounts: merge deduplication and pass-through counts
DedupCommand->>DeduplicationMetrics: convert aggregate counts
DeduplicationMetrics->>MetricsFile: write TSV metrics
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
Comment |
Member
Author
|
@coderabbitai pause |
✅ Action performedReviews paused. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #740 +/- ##
==========================================
+ Coverage 94.12% 94.18% +0.06%
==========================================
Files 181 186 +5
Lines 109415 110929 +1514
==========================================
+ Hits 102987 104482 +1495
- Misses 6428 6447 +19 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Member
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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-raw-bam/src/tags.rs`:
- Line 155: Update extract_int_value to compute every offset and required byte
range with checked arithmetic, including the initial position and type-specific
widths. Return None whenever an addition or bounds calculation overflows or
exceeds aux_data, preventing wrapped offsets from being decoded or indexed.
- Around line 151-153: Update the rustdoc in extract_int_value’s shared-helper
comment to render find_mi_tag as inline code. In extract_int_value, replace
unchecked position arithmetic for p + 3, p + 4, and p + 6 with checked bounds
calculations, returning failure when any required range overflows or exceeds the
buffer.
In `@src/lib/commands/dedup.rs`:
- Around line 834-841: Update the metrics documentation in the deduplication
command, including the comment near the reported columns and the field docs for
total_templates and related wording, to describe total_templates as “templates
that passed the filter” rather than “templates written.” Preserve the
reconciliation invariant and avoid changing counting behavior.
- Around line 733-751: The pass-through loop over passthrough_templates must
apply the same tc-tag validation as the main counting loop before updating
dedup_counts. Reuse the existing check and hard-failure behavior for every
record in each template, including secondary and supplementary records, so
--include-unmapped cannot bypass it.
🪄 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: 26c68b0f-1784-49d9-8ba5-e564c7e16d1d
⛔ Files ignored due to path filters (1)
CHANGELOG.mdis excluded by!**/CHANGELOG.md
📒 Files selected for processing (16)
.coderabbit.yamlcrates/fgumi-metrics/src/dedup.rscrates/fgumi-metrics/src/group.rscrates/fgumi-metrics/src/lib.rscrates/fgumi-metrics/src/template_filter.rscrates/fgumi-raw-bam/src/lib.rscrates/fgumi-raw-bam/src/tags.rsdocs/src/guide/working-with-metrics.mdsrc/lib/commands/dedup.rssrc/lib/commands/group.rssrc/lib/grouper.rssrc/lib/metrics/mod.rssrc/lib/mod.rssrc/lib/template_filter.rstests/integration/test_dedup_command.rstests/integration/test_group_command.rs
The template filter's per-reason counts were collected, merged across workers, and then dropped: DedupMetricsOutput had no field for them. A run that filtered out every input template reported a row of zeros with no indication anything had been dropped, and logged only 'Deduplication complete: 0 templates'. Serialize them, log a per-reason summary so --metrics is not the only channel, and assert the reconciliation filtered_templates + total_templates == templates read, so a future column cannot go missing the same way. BREAKING CHANGE: dedup --metrics gains eleven columns. Consumers parsing it positionally must be updated; consumers keyed on column names are unaffected. Closes #739
The two implementations were a copy-paste pair. A normalized diff showed them semantically identical except for a single condition: group gated the fully-unmapped rejection on --allow-unmapped, dedup always rejected. That is now TemplateFilterConfig::allow_unmapped, which dedup sets to false because its --include-unmapped pass-throughs are split off before the filter runs. Keeping them separate was already a liability, and the reason-recording change made it worse: nine rejection sites each that had to stay in sync. While consolidating, reuse what the raw-BAM crate already owns rather than carrying new copies: - Drop a second copy of fgumi-raw-bam's integer-aux decode ladder and publish that crate's extract_int_value instead. Reusing find_int_tag would have been the obvious call but costs a second scan of the aux block per primary read; taking the decoder alone keeps the single pass. - Use RawRecord::is_unmapped / is_qc_fail / is_mate_unmapped instead of hand-rolled flag masks, which also removes two RawRecordView constructions from the per-read loops. - Extract template_has_malformed_record and template_is_fully_unmapped, and have dedup's --include-unmapped pass-through check call them. It had been re-deriving this function's first three branches with inverted polarity, so the two could drift on what malformed and unmapped mean. - Extract the aux-block scan into scan_aux_for_mq_and_umi; the merged function was long enough to trip clippy::too_many_lines, and the scan is a self-contained parser that reads better named than inlined. It returns early when neither tag is wanted. - Turn set_filter_counts into the from_filter_counts constructor: it was only ever called on a freshly defaulted struct, so its four zeroing assignments were dead, and 'from' is the repo's convention for factories. - Give TemplateFilterConfig a hand-written Default (a derived one would produce an invalid all-zero umi_tag), collapsing 18 six-field literals in tests. - ProcessedDedupGroup::estimate_heap_size no longer adds size_of::<DedupCounts>(); the counts are inline, not heap, and the term inflated the queue's memory estimate for small groups. Both commands' existing filter tests stay where they are: they now exercise the shared function through each command's own fixtures, which is broader coverage than a single merged table. The new test in template_filter covers allow_unmapped, the one branch that distinguishes the callers. group's .grouping_metrics.txt re-verified byte-identical across three filter regimes; dedup's metrics unchanged on the same input.
The --help text has promised per-reason drop counts since #565 without the code ever writing them. Correct it to name the actual columns, their unit, and the reconciliation, and note that total_templates counts templates written rather than read. Its list of drop reasons was also missing the no-primary-reads case, which left one filtered_* column with nothing documenting it. Metric field doc comments become the Description column of the generated reference page, so they stay one short line each; the prose about units and reconciliation lives in the struct doc, which renders as page text. Also flag in the metrics guide that dedup counts discards in templates while group counts them in primary records, so the two files are not comparable. The DeduplicationMetrics reference page is generated by cargo xtask from those doc comments; that output is gitignored and built in CI, so nothing is committed for it here. Two loose ends from reviewing the above: assert passthrough_templates and filtered_unmapped in test_dedup_include_unmapped, which previously checked record counts only and passed no --metrics, leaving that column's production increment unverified; and aim the grouping-logic path instruction at the new template_filter module, since the existing glob expands to template.rs only.
nh13
force-pushed
the
739/nhomer/template-filter-metrics
branch
from
August 12, 2026 16:08
1b31f57 to
f36659d
Compare
Member
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
This was referenced Aug 12, 2026
Merged
Merged
This branch was previously 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
1 participant
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.
Closes #739.
fgumi dedupis a read filter as well as a duplicate marker: templates that fail the pre-grouping filter are dropped and never reach the output. It counted why it dropped each one, merged those counts across workers — and then discarded them at the serialization boundary, becauseDedupMetricsOutputhad no field to hold them. A run that filtered out its entire input wrote a row of zeros and loggedDeduplication complete: 0 templates, with nothing to indicate records had been dropped or why.Reproducer on 574 simulated templates, all failing
-q 61, before this change:After:
plus a new log line —
Filtered out 574 templates before marking: 574 low_mapping_quality— so the drops are visible even without--metrics.Why the fix is not just "add the missing columns"
grouper::FilterMetricswasgroup's fgbio output schema being used as a measurement primitive. Its fourdiscarded_*buckets are named after fgbio'sUmiGroupingMetriccolumns, down to preserving fgbio'sdiscarded_umis_to_shorttypo. That is a column vocabulary, not a reason vocabulary, and the filter has nine distinct rejection sites — so five of them collapsed intodiscarded_poor_alignment, including missing-RXand truncated-record, which are not alignment problems.dedupborrowed the struct and inherited both the vocabulary and a unit mismatch:groupincrements it by primary-record count,dedupby one per template, with nothing in the type to tell them apart. The field namedaccepted_templateshas been holding a record count ingroupall along.So the three collapsed layers are separated:
TemplateFilterReason— one variant per rejection site in the code, not per output column.TemplateFilterCounts— a collector keyed by that enum, recording every decision in both template and primary-record units, so the unit is an explicit rendering choice rather than an implicit property of a sharedu64.groupprojects the nine reasons back onto fgbio's four columns in record units;dedupgets eleven newfiltered_*columns in template units.group's output is unchangedThis is the hard constraint on the change and it is enforced, not asserted. Golden
.grouping_metrics.txtfiles were captured from the pre-change binary across three filter regimes (nothing filtered, all filtered for mapping quality, all filtered for UMI length, so more than one fgbio column is exercised) and diffed after every subsequent phase. All byte-identical. A characterization test pinning the five-column header at the command level was committed before the refactor so it could catch a regression during it, andUmiGroupingMetrics::from_filter_countsroutes through an exhaustive match, so adding a tenth reason is a compile error until someone assigns it a column.Breaking change
dedup --metricsgains eleven columns. Consumers keyed on column names are unaffected; consumers parsing positionally must be updated. Note thatfgumi compare metricstreats a column-set difference asDIFFER, so comparing a pre- and post-change dedup metrics file will fail by design.Suggested reading order
feat(metrics): add reason-keyed template filter accounting— the primitive, in isolation.refactor: record filter rejections by reason in group and dedup— the migration. Both commands convert together because deletingFilterMetricsbreaksdedup, and every commit builds.fix(dedup)!: report filtered template counts in the metrics file— the actual fix.refactor: share one template filter between group and dedup— the largest diff, and optional to the fix. A normalized diff of the two filter implementations showed them semantically identical except for one condition (allow_unmapped); they are now one function.The rest are a mechanical rename, the new metrics type, and docs.
Notes for the reviewer
test_filter_template_raw_truncated_record_no_panicwas recorded as a poor-alignment rejection, but the record has a truncated aux block, clears the length check, and is rejected for a missing UMI. Its own comment said so. The coarse bucket had been hiding the discrepancy.filtered_templates + total_templates == templates readis asserted in unit tests and end-to-end. That is the test that would have caught the original bug.total_templatesdeliberately keeps its name and means "templates written". Adding columns without renaming existing ones keeps name-keyed consumers working; the doc comment says so explicitly.input_templatescolumn — it is derivable, matching howUmiGroupingMetricstreats its derivable fields.docs/src/metrics/deduplication-metrics.mdis generated bycargo xtaskand gitignored, so it does not appear in the diff. Moving the schema intofgumi-metricsis what makes it generate at all;dedupwas previously the only metric type with no reference page.Verification
cargo ci-test(7,277 tests),cargo ci-lint,cargo ci-fmtall clean.groupgoldens re-diffed after every phase including the final cleanup pass; the #739 reproducer re-run against the final binary.Risk:
dedup --metricsoutput changes, pinned by exact schema and reconciliation tests;unsafe: none and the CLAUDE.md allowlist is unchanged; memory, queue, and backpressure policy: none.Fixes
fgumi dedup --metricsso filtered templates and rejection reasons are serialized. Adds elevenfiltered_*columns and passthrough counts. Preservesgroup’s five-column fgbio-compatible output.Shares template filtering and accounting between
groupanddedup. Adds coverage for filter reasons, unmapped pass-through, output reconciliation, and metrics column order.Documents metric units, reconciliation rules, and the positional parsing breaking change.