Repository navigation
refactor(metrics): improve code reuse, quality, and efficiency in fgumi-metrics - #130
Merged
Merged
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #130 +/- ##
==========================================
+ Coverage 83.50% 83.61% +0.10%
==========================================
Files 126 126
Lines 51375 51516 +141
==========================================
+ Hits 42901 43074 +173
+ Misses 8474 8442 -32 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (10)
📝 WalkthroughWalkthroughThis change refactors the metrics crate to centralize data aggregation and improve API consistency. ClipCounts bundles per-operation clipping counts into a single struct, replacing discrete parameters in the clipping metrics update method. UmiCountTracker consolidates per-UMI tracking across multiple HashMaps. Rejection handling now uses constant arrays to organize core, optional, and all rejection reasons. Helper functions frac() and frac_u64() provide standardized division-by-zero-safe fraction computation. New RejectionReason methods tsv_key() and kv_description() support metric key generation. Writer module gains read_metrics and read_metrics_auto functions for symmetric read/write operations. UmiGroupingMetrics now derives Default. Updates propagate these changes through command handlers and public re-exports. 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
crates/fgumi-metrics/src/clip.rs (2)
557-573: Add oneproptestfor+=invariants.The example test is solid; a property test would protect additive behavior across many inputs.
Proposed property test
#[cfg(test)] mod tests { use super::*; use fgumi_sam::builder::RecordBuilder; + use proptest::prelude::*; @@ + proptest! { + #[test] + fn prop_add_assign_sums_reads_and_bases( + r1 in 0usize..10_000, + r2 in 0usize..10_000, + b1 in 0usize..100_000, + b2 in 0usize..100_000, + ) { + let mut left = ClippingMetrics::new(ReadType::All); + left.reads = r1; + left.bases = b1; + + let mut right = ClippingMetrics::new(ReadType::All); + right.reads = r2; + right.bases = b2; + + left += &right; + prop_assert_eq!(left.reads, r1 + r2); + prop_assert_eq!(left.bases, b1 + b2); + } + } }As per coding guidelines, "Use proptest for property-based testing".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/fgumi-metrics/src/clip.rs` around lines 557 - 573, Add a proptest that verifies the additive invariants of ClippingMetrics when using the += operator: generate random ReadType, two random records (or lengths) and random ClipCounts, build metrics_a and metrics_b via ClippingMetrics::new and update(...) calls, then assert that metrics_a.clone() += &metrics_b yields the same result as metrics_b.clone() += &metrics_a (commutativity) and that metrics_a += &metrics_b equals the metrics produced by creating an empty ClippingMetrics and performing both updates sequentially (consistency with sequential updates); place this proptest alongside the existing test_clipping_metrics_add_assign and reference ClippingMetrics, update, ClipCounts, ReadType, and the += operator in the test.
331-380: Refactor repetitive clip-variant tests intorstestcases.These tests are the same shape and are easier to extend as table-driven cases.
Proposed refactor
#[cfg(test)] mod tests { use super::*; use fgumi_sam::builder::RecordBuilder; + use rstest::rstest; @@ - #[test] - fn test_clipping_metrics_update_five_prime() { - let mut metrics = ClippingMetrics::new(ReadType::ReadOne); - let record = create_test_record("95M", false); - metrics.update(&record, ClipCounts { five_prime: 5, ..ClipCounts::default() }); - assert_eq!(metrics.reads_clipped_five_prime, 1); - assert_eq!(metrics.bases_clipped_five_prime, 5); - assert_eq!(metrics.reads_clipped_post, 1); - assert_eq!(metrics.bases_clipped_post, 5); - } - - #[test] - fn test_clipping_metrics_update_three_prime() { /* ... */ } - #[test] - fn test_clipping_metrics_update_overlapping() { /* ... */ } - #[test] - fn test_clipping_metrics_update_extending() { /* ... */ } + #[rstest] + #[case(ClipCounts { prior: 0, five_prime: 5, three_prime: 0, overlapping: 0, extending: 0 }, 1, 0, 0, 0, 5)] + #[case(ClipCounts { prior: 0, five_prime: 0, three_prime: 3, overlapping: 0, extending: 0 }, 0, 1, 0, 0, 3)] + #[case(ClipCounts { prior: 0, five_prime: 0, three_prime: 0, overlapping: 20, extending: 0 }, 0, 0, 1, 0, 20)] + #[case(ClipCounts { prior: 0, five_prime: 0, three_prime: 0, overlapping: 0, extending: 15 }, 0, 0, 0, 1, 15)] + fn test_clipping_metrics_update_single_clip_type( + #[case] counts: ClipCounts, + #[case] exp_five: usize, + #[case] exp_three: usize, + #[case] exp_overlap: usize, + #[case] exp_extend: usize, + #[case] exp_post_bases: usize, + ) { + let mut metrics = ClippingMetrics::new(ReadType::ReadOne); + let record = create_test_record("100M", false); + metrics.update(&record, counts); + assert_eq!(metrics.reads_clipped_five_prime, exp_five); + assert_eq!(metrics.reads_clipped_three_prime, exp_three); + assert_eq!(metrics.reads_clipped_overlapping, exp_overlap); + assert_eq!(metrics.reads_clipped_extending, exp_extend); + assert_eq!(metrics.bases_clipped_post, exp_post_bases); + } }As per coding guidelines, "Use rstest for parameterized tests".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/fgumi-metrics/src/clip.rs` around lines 331 - 380, Refactor the four repetitive tests (test_clipping_metrics_update_five_prime, test_clipping_metrics_update_three_prime, test_clipping_metrics_update_overlapping, test_clipping_metrics_update_extending) into a single parameterized rstest that drives ClippingMetrics::new(ReadType::ReadOne), create_test_record, and metrics.update(&record, ClipCounts { ... }) with a table of variants (five_prime, three_prime, overlapping, extending) and expected counters; for each case assert the corresponding reads_clipped_* and bases_clipped_* fields plus reads_clipped_post and bases_clipped_post match the expected values; reference the ClipCounts fields (five_prime, three_prime, overlapping, extending), ClippingMetrics::update, and the existing create_test_record helper when building the rstest cases.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@crates/fgumi-metrics/src/clip.rs`:
- Around line 557-573: Add a proptest that verifies the additive invariants of
ClippingMetrics when using the += operator: generate random ReadType, two random
records (or lengths) and random ClipCounts, build metrics_a and metrics_b via
ClippingMetrics::new and update(...) calls, then assert that metrics_a.clone()
+= &metrics_b yields the same result as metrics_b.clone() += &metrics_a
(commutativity) and that metrics_a += &metrics_b equals the metrics produced by
creating an empty ClippingMetrics and performing both updates sequentially
(consistency with sequential updates); place this proptest alongside the
existing test_clipping_metrics_add_assign and reference ClippingMetrics, update,
ClipCounts, ReadType, and the += operator in the test.
- Around line 331-380: Refactor the four repetitive tests
(test_clipping_metrics_update_five_prime,
test_clipping_metrics_update_three_prime,
test_clipping_metrics_update_overlapping,
test_clipping_metrics_update_extending) into a single parameterized rstest that
drives ClippingMetrics::new(ReadType::ReadOne), create_test_record, and
metrics.update(&record, ClipCounts { ... }) with a table of variants
(five_prime, three_prime, overlapping, extending) and expected counters; for
each case assert the corresponding reads_clipped_* and bases_clipped_* fields
plus reads_clipped_post and bases_clipped_post match the expected values;
reference the ClipCounts fields (five_prime, three_prime, overlapping,
extending), ClippingMetrics::update, and the existing create_test_record helper
when building the rstest cases.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
crates/fgumi-metrics/src/clip.rscrates/fgumi-metrics/src/consensus.rscrates/fgumi-metrics/src/correct.rscrates/fgumi-metrics/src/duplex.rscrates/fgumi-metrics/src/group.rscrates/fgumi-metrics/src/lib.rscrates/fgumi-metrics/src/rejection.rscrates/fgumi-metrics/src/writer.rssrc/commands/clip.rssrc/lib/metrics/mod.rs
…mi-metrics Add frac()/frac_u64() safe division utilities to eliminate 9+ copies of the inline division-with-zero-check pattern. Extract UmiCountTracker to replace 6 parallel HashMaps in DuplexMetricsCollector. Introduce ClipCounts struct to bundle 5 clipping parameters. Add tsv_key() and kv_description() to RejectionReason with constant arrays and rejection_count() helper to simplify consensus metrics methods. Add generic read_metrics()/read_metrics_auto() to writer module and delegate from UmiCorrectionMetrics. Derive Default for UmiGroupingMetrics, implement AddAssign for ClippingMetrics, return fixed-size array from all_metrics(), and optimize format_count() with direct string building.
nh13
force-pushed
the
refactor/nh/metrics-crate-improvements
branch
from
February 28, 2026 22:59
bf2db79 to
d8fc0f8
Compare
This was referenced Feb 28, 2026
Closed
Merged
This was referenced Jul 10, 2026
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.
Summary
frac()/frac_u64()safe division utilities to centralize the repeated zero-denominator-check pattern (9+ call sites)UmiCountTrackerstruct to replace 6 parallelHashMapfields inDuplexMetricsCollector, eliminating redundant.keys()+.get()lookupsClipCountsstruct to bundle 5 clipping parameters into a single argumenttsv_key()/kv_description()methods toRejectionReasonwith constant arrays andrejection_count()helper, simplifyingtotal_rejections(),rejection_summary(),push_optional_rejections(), andto_kv_metrics()read_metrics()/read_metrics_auto()towritermodule, delegate fromUmiCorrectionMetricsDefaultforUmiGroupingMetrics, implementAddAssignforClippingMetrics, return[&ClippingMetrics; 5]fromall_metrics(), optimizeformat_count()with direct string buildingTest plan
cargo ci-test)cargo ci-fmt)cargo ci-lint)frac()/frac_u64()edge casesUmiCountTrackerrecord/iter/emptyClipCountsdefaultAddAssignonClippingMetricsRejectionReason::tsv_key()andkv_description()read_metrics()/read_metrics_auto()roundtrip and error handling