Repository navigation
feat: add simplex-metrics command for simplex sequencing QC - #195
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #195 +/- ##
==========================================
+ Coverage 86.12% 87.90% +1.77%
==========================================
Files 110 113 +3
Lines 51974 52593 +619
==========================================
+ Hits 44765 46231 +1466
+ Misses 7209 6362 -847 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (15)
✅ Files skipped from review due to trivial changes (3)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthroughAdds a new 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 review |
✅ Actions performedReview triggered.
|
6772f11 to
4adb79a
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
tests/integration/test_simplex_metrics_command.rs (2)
199-204: Direct index access could panic if yield metrics count changes.
metrics[19]assumes exactly 20 entries. IfDOWNSAMPLING_FRACTIONSchanges, this panics without a helpful message.🔧 Suggested improvement
- let full = &metrics[19]; + let full = metrics.last().expect("Should have at least one yield metric");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/integration/test_simplex_metrics_command.rs` around lines 199 - 204, The test uses a hard-coded index metrics[19] which will panic if the number of entries or DOWNSAMPLING_FRACTIONS changes; update the assertion to locate the 100% entry robustly by searching metrics for an item with fraction approximately 1.0 (e.g., metrics.iter().find(|m| (m.fraction - 1.0).abs() < EPS)) or use metrics.last() with a clear unwrap message, then assert on ss_families and cs_families using that found entry (reference symbols: metrics, fraction, ss_families, cs_families in test_simplex_metrics_command.rs).
283-290: Same direct index pattern here.Use
.last()or.find()for robustness, consistent with the suggested pattern above.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/integration/test_simplex_metrics_command.rs` around lines 283 - 290, The test uses brittle direct indexing (&default_metrics[19], &strict_metrics[19]) to get the "full" metric; replace these with calls that pick the last element for robustness, e.g. use default_metrics.last().expect("expected default_metrics non-empty") and strict_metrics.last().expect("expected strict_metrics non-empty") (or .last().unwrap()) and update default_full and strict_full to bind those results instead of indexing so the assertions that follow remain valid.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@resources/CollectSimplexSeqMetrics.R`:
- Line 46: The assignment to sampleInfo uses paste(args[5:length(args)]) which
returns a vector when multiple args are present; change the call in the
sampleInfo assignment to paste(args[5:length(args)], collapse=" ") so all
elements from args[5] onward are concatenated into a single string for the title
(refer to the sampleInfo variable and the paste(...) call).
In `@src/commands/shared_metrics.rs`:
- Around line 141-150: compute_hash_fraction can panic when `hash as i32` equals
i32::MIN because calling `.abs()` on i32::MIN panics in debug; update
compute_hash_fraction to detect i32::MIN before calling .abs(): cast the hash to
i32 into `hash_i32`, if `hash_i32 == i32::MIN` compute the positive value as
2147483648 (e.g. (i32::MAX as i64 + 1) as f64) otherwise use `hash_i32.abs()`
safely (promoting to i64 before abs to avoid overflow), then divide that f64
numerator by `i32::MAX as f64` to preserve the original normalization behavior.
---
Nitpick comments:
In `@tests/integration/test_simplex_metrics_command.rs`:
- Around line 199-204: The test uses a hard-coded index metrics[19] which will
panic if the number of entries or DOWNSAMPLING_FRACTIONS changes; update the
assertion to locate the 100% entry robustly by searching metrics for an item
with fraction approximately 1.0 (e.g., metrics.iter().find(|m| (m.fraction -
1.0).abs() < EPS)) or use metrics.last() with a clear unwrap message, then
assert on ss_families and cs_families using that found entry (reference symbols:
metrics, fraction, ss_families, cs_families in test_simplex_metrics_command.rs).
- Around line 283-290: The test uses brittle direct indexing
(&default_metrics[19], &strict_metrics[19]) to get the "full" metric; replace
these with calls that pick the last element for robustness, e.g. use
default_metrics.last().expect("expected default_metrics non-empty") and
strict_metrics.last().expect("expected strict_metrics non-empty") (or
.last().unwrap()) and update default_full and strict_full to bind those results
instead of indexing so the assertions that follow remain valid.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5880082b-e429-474d-9dc1-e15023b5df8b
📒 Files selected for processing (14)
crates/fgumi-metrics/src/duplex.rscrates/fgumi-metrics/src/lib.rscrates/fgumi-metrics/src/shared.rscrates/fgumi-metrics/src/simplex.rsresources/CollectSimplexSeqMetrics.Rsrc/commands/duplex_metrics.rssrc/commands/mod.rssrc/commands/shared_metrics.rssrc/commands/simplex_metrics.rssrc/lib/metrics/mod.rssrc/main.rstests/integration/main.rstests/integration/test_duplex_metrics_command.rstests/integration/test_simplex_metrics_command.rs
…cing QC Add a new `simplex-metrics` command that collects comprehensive QC metrics for simplex sequencing experiments, mirroring `duplex-metrics` but adapted for simplex-only data (no DS/duplex columns). Outputs: - Family size distributions (CS and SS families) - Yield curves at 20 downsampling levels (5%-100%) - UMI observation frequencies - Optional PDF plots via embedded R script (7 ggplot2 visualizations) Also refactors ~800 lines of shared infrastructure out of `duplex-metrics` into `shared_metrics.rs` and `shared.rs` for reuse by both commands. Closes #192
4adb79a to
bbf7b06
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Summary
Implements
simplex-metrics(closes #192) — a new command for collecting QC metrics on simplex sequencing data, mirroring the existingduplex-metricscommand but adapted for simplex-only experiments.duplex-metricsintoshared_metrics.rsandshared.rsfor reuse by both commandsTest plan
cargo ci-fmtandcargo ci-lintpass