Repository navigation
fix(codec): emit fgbio KV metrics format (thread-consistent, seeded rows) - #513
Conversation
|
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 ignored due to path filters (1)
📒 Files selected for processing (3)
Walkthrough
ChangesRejection and metrics alignment
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Codec as Codec execute path
participant Finalize as finalize_stats
participant Metrics as CodecConsensusStats metrics
participant StatsFile as KV stats TSV
Codec->>Finalize: finalize merged consensus statistics
Finalize->>Metrics: convert to standard metrics
Finalize->>StatsFile: write seeded fgbio KV rows
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #513 +/- ##
=======================================
Coverage 92.61% 92.62%
=======================================
Files 166 166
Lines 99803 99793 -10
=======================================
- Hits 92431 92429 -2
+ Misses 7372 7364 -8 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
2119c7f to
fb4e5e2
Compare
e80c30b to
37e2e83
Compare
fb4e5e2 to
e9ce631
Compare
37e2e83 to
d52374b
Compare
e9ce631 to
5f41a16
Compare
0fe810a to
ae3dee2
Compare
|
@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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
crates/fgumi-metrics/src/rejection.rs (1)
186-244: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winManually-maintained
ALL_REASONSarray risks silent test-coverage drift.
ALL_REASONSmust be updated by hand every time a variant is added; nothing forces it to stay in sync (unlikedescription()/tsv_key(), which are compiler-enforced via exhaustivematch). A forgotten update silently drops coverage fromtest_tsv_key_prefixandtest_kv_description_non_empty.Prefer
strum::EnumIterto generate the reason list automatically, and userstestto parameterize these two tests over it, per the repo's Rust test guidelines.Based on coding guidelines: "Use
rstestfor parameterized tests in Rust test files."🤖 Prompt for 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. In `@crates/fgumi-metrics/src/rejection.rs` around lines 186 - 244, Replace the manually maintained ALL_REASONS array with strum::EnumIter-based iteration over RejectionReason, ensuring every enum variant is included automatically. Update test_tsv_key_prefix and test_kv_description_non_empty to use rstest parameterization over the generated variants, while preserving their existing assertions and keeping test_duplex_row_keys_match_fgbio unchanged.Source: Coding guidelines
src/lib/commands/codec.rs (1)
1400-1415: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winParity test checks record count + CB presence, not record identity.
records_st/records_mtare already materialized; assert byte/field identity (name, seq, quals) so single- vs multi-threaded content divergence is caught, not just count. As per path instructions: "end-to-end tests that assert a record count but not record identity" should be flagged.🤖 Prompt for 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. In `@src/lib/commands/codec.rs` around lines 1400 - 1415, Extend the parity assertions around records_st and records_mt to compare each corresponding record’s identity fields, including name, sequence, and qualities, in addition to count and CB-tag presence. Use the already materialized records and preserve the existing single-threaded versus multi-threaded comparison structure so content divergence is detected.Source: Path instructions
🤖 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/duplex_caller.rs`:
- Around line 2368-2434: Add a test alongside the existing fragment rejection
tests that builds one MI group containing fragment reads plus valid AB/BA pairs
using the existing ab_r1, ab_r2, ba_r1, and ba_r2 helpers. Call consensus_reads
and assert fragments increment RejectionReason::FragmentRead (mapping to
NonPairedReads), while the paired inputs still produce the duplex consensus with
result.count == 2.
---
Outside diff comments:
In `@crates/fgumi-metrics/src/rejection.rs`:
- Around line 186-244: Replace the manually maintained ALL_REASONS array with
strum::EnumIter-based iteration over RejectionReason, ensuring every enum
variant is included automatically. Update test_tsv_key_prefix and
test_kv_description_non_empty to use rstest parameterization over the generated
variants, while preserving their existing assertions and keeping
test_duplex_row_keys_match_fgbio unchanged.
In `@src/lib/commands/codec.rs`:
- Around line 1400-1415: Extend the parity assertions around records_st and
records_mt to compare each corresponding record’s identity fields, including
name, sequence, and qualities, in addition to count and CB-tag presence. Use the
already materialized records and preserve the existing single-threaded versus
multi-threaded comparison structure so content divergence is detected.
🪄 Autofix (Beta)
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: 18d309c0-a61c-4696-9464-8e9d570699a9
📒 Files selected for processing (9)
crates/fgumi-consensus/src/caller.rscrates/fgumi-consensus/src/duplex_caller.rscrates/fgumi-metrics/src/consensus.rscrates/fgumi-metrics/src/lib.rscrates/fgumi-metrics/src/rejection.rssrc/lib/commands/codec.rssrc/lib/commands/duplex.rssrc/lib/commands/simplex.rstests/integration/test_duplex_command.rs
ae3dee2 to
4332216
Compare
5f41a16 to
84dbee9
Compare
|
Addressed the two outside-diff-range review comments in the latest push (also rebased onto the updated base branch, so this PR is now a clean single commit on top of it):
The inline comment about adding a mixed fragment + valid AB/BA duplex group test is covered by |
|
@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-metrics/Cargo.toml`:
- Line 20: Move strum from [dependencies] to [dev-dependencies] in the crate
manifest, preserving its derive feature, since rejection.rs uses it only under
test configuration and within mod tests.
In `@src/lib/commands/codec.rs`:
- Around line 470-479: Extract the duplicated statistics-writing logic from the
single- and multi-threaded paths into a shared helper on the relevant command
type, using the existing stats options, metrics conversion, DelimFile writing,
context, and info logging. Replace both inline blocks with calls to that helper
so both paths remain behaviorally identical.
🪄 Autofix (Beta)
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: 096c232c-219e-4ef4-bdc0-1da1608f7b4c
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock,!**/*.lock
📒 Files selected for processing (3)
crates/fgumi-metrics/Cargo.tomlcrates/fgumi-metrics/src/rejection.rssrc/lib/commands/codec.rs
5928bfa to
c9c8f07
Compare
c9c8f07 to
fc11325
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@src/lib/commands/codec.rs`:
- Around line 999-1041: Parameterize
test_codec_execute_rejects_non_template_coordinate_input with rstest to run
under both ThreadingOptions::none() and the threaded mode used by
test_threading_modes. Pass the selected threading configuration into
create_codec_with_paths so each execute path, including execute_threads_mode and
the fast path, validates non-template-coordinate input and preserves the
existing error assertion.
🪄 Autofix (Beta)
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: 20c7204d-60b5-4a71-b838-834747971140
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock,!**/*.lock
📒 Files selected for processing (3)
crates/fgumi-metrics/Cargo.tomlcrates/fgumi-metrics/src/rejection.rssrc/lib/commands/codec.rs
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 1
🤖 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 `@src/lib/commands/codec.rs`:
- Around line 999-1041: Parameterize
test_codec_execute_rejects_non_template_coordinate_input with rstest to run
under both ThreadingOptions::none() and the threaded mode used by
test_threading_modes. Pass the selected threading configuration into
create_codec_with_paths so each execute path, including execute_threads_mode and
the fast path, validates non-template-coordinate input and preserves the
existing error assertion.
🪄 Autofix (Beta)
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: 20c7204d-60b5-4a71-b838-834747971140
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock,!**/*.lock
📒 Files selected for processing (3)
crates/fgumi-metrics/Cargo.tomlcrates/fgumi-metrics/src/rejection.rssrc/lib/commands/codec.rs
🛑 Comments failed to post (1)
src/lib/commands/codec.rs (1)
999-1041: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win
Negative sort-order test only covers the single-threaded fast path.
check_consensus_sort_orderis duplicated verbatim in both the fast path (line 342-345) andexecute_threads_mode's caller (line 315-318), buttest_codec_execute_rejects_non_template_coordinate_inputonly runs with default (ThreadingOptions::none()) threading. A regression that drops/breaks the threaded-path check wouldn't be caught. Parameterize with#[rstest]over both threading modes, mirroringtest_threading_modes.As per coding guidelines, "Use
rstestfor parameterized tests in Rust test files."♻️ Proposed fix
- #[test] - fn test_codec_execute_rejects_non_template_coordinate_input() -> Result<()> { + #[rstest] + #[case::fast_path(ThreadingOptions::none())] + #[case::threaded(ThreadingOptions::new(2))] + fn test_codec_execute_rejects_non_template_coordinate_input( + #[case] threading: ThreadingOptions, + ) -> Result<()> { // CONS-01: codec requires template-coordinate-sorted input... ... - let cmd = create_codec_with_paths(input_path, output_path); + let mut cmd = create_codec_with_paths(input_path, output_path); + cmd.threading = threading; let err = cmd.execute("test").expect_err("codec must reject non-template-coordinate input");Also applies to: 310-318, 340-345
🤖 Prompt for 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. In `@src/lib/commands/codec.rs` around lines 999 - 1041, Parameterize test_codec_execute_rejects_non_template_coordinate_input with rstest to run under both ThreadingOptions::none() and the threaded mode used by test_threading_modes. Pass the selected threading configuration into create_codec_with_paths so each execute path, including execute_threads_mode and the fast path, validates non-template-coordinate input and preserves the existing error assertion.Source: Coding guidelines
The `codec` command wrote its statistics as the wide `ConsensusMetrics` table (`write_tsv([metrics])`) in both the single- and multi-threaded paths, unlike `simplex` and `duplex` which emit fgbio's vertical key/value/description format (`ConsensusKvMetric`). The wide format is not readable by fgbio's `Metric.read` and does not carry the `raw_reads_rejected_for_*` rejection rows in a fgbio-parseable shape. Convert both codec paths to `to_kv_metrics_seeded`, seeding the `usedByCodec` rejection reasons fgumi models (`non_paired_reads`, `single_strand_only`, `potential_umi_collision`) via a new `CODEC_SEEDED_REJECTIONS`. Codec is a duplex-style caller, so it shares the duplex-seeded set. The metrics output was already thread-consistent for codec (both paths use the same caller stats); this keeps that and adds a KV-format assertion to the codec threading-parity test. Note: fgbio additionally seeds codec-specific rejection reasons (`r1_r2_overlap_too_short`, `indel_error_between_strands`, `high_duplex_disagreement`, `clip_overlap_failed`, `not_primary_fr_pair`) that fgumi does not yet model as distinct `RejectionReason`s — a separate follow-up. Stacked on #511 (uses its `to_kv_metrics_seeded` + rejection-reason work).
fc11325 to
183cffb
Compare
|
Addressed the outside-diff finding on the negative sort-order test (it had no inline thread — "comments failed to post" on the last review).
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
…585) test_codec_allow_unmapped_gates_pregroup_filter parsed the codec stats file as a wide ConsensusMetrics TSV, but codec emits fgbio's vertical key-value format (ConsensusKvMetric). The header mismatch made read_tsv fail with "Error parsing delimited data file header" in all four cases. The break was a semantic merge conflict: #513 switched codec stats to the KV format, then #509 (merged after) added this test using the old wide layout. Both merged cleanly textually, so the failure only surfaced on the merged main tip. Read the rows back as ConsensusKvMetric and look up the counts by key (raw_reads_considered, consensus_reads_emitted), mirroring the sibling simplex tests. The assertions' intent is unchanged: the supplementary alignment is filtered before grouping (2 raw reads considered) and the single FR molecule is emitted (1 consensus read).
This branch was previously deployed
1 participant
Summary
codecwrote its statistics as the wideConsensusMetricstable in both the single- and multi-threaded paths, unlikesimplexandduplexwhich emit fgbio's vertical key/value/description format (ConsensusKvMetric). The wide format is not readable by fgbio'sMetric.readand does not carry theraw_reads_rejected_for_*rejection rows in a fgbio-parseable shape.This converts both codec paths to
to_kv_metrics_seeded, so codec now emits the same fgbio KV format as the other consensus tools.Stacked on #511 (
nh/fix-consensus-rejection-parity) — it uses that PR'sto_kv_metrics_seededand rejection-reason work. Merge order: #504 → #511 → this.What changed
executeandexecute_threads_mode) now writeto_kv_metrics_seeded(&CODEC_SEEDED_REJECTIONS).CODEC_SEEDED_REJECTIONSseeds theusedByCodecrejection reasons fgumi models (non_paired_reads,single_strand_only,potential_umi_collision) — codec is a duplex-style caller, so it shares the duplex-seeded set.raw_reads_rejected_for_*row) in addition to the existing single==threaded equality check.Codec's metrics were already thread-consistent (both paths use the same caller stats), so no value changes were needed — only the output format.
Not addressed (follow-ups)
r1_r2_overlap_too_short,indel_error_between_strands,high_duplex_disagreement,clip_overlap_failed,not_primary_fr_pair) that fgumi does not yet model as distinctRejectionReasons.consensus_reads_emittedcounts emitted reads vs. consensus events (the analogue of the duplex fix in fix(consensus): fgbio rejection-reason parity + thread-independent duplex metrics #511) was not audited here — it is consistent across thread counts, which is what this PR targets.Testing
ci-fmtandci-lintclean.Summary by CodeRabbit
New Features
Bug Fixes
Tests