Repository navigation
feat(compare): restore --command preset for per-stage compare defaults - #307
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #307 +/- ##
==========================================
- Coverage 90.47% 90.46% -0.01%
==========================================
Files 101 101
Lines 59992 60059 +67
==========================================
+ Hits 54276 54331 +55
- Misses 5716 5728 +12 ☔ 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 (1)
📝 WalkthroughWalkthroughThe change introduces a 🚥 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.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
src/lib/commands/compare/bams.rs (1)
958-964: Logged preset name usesDebugcasing.
"{:?}"onCommandPreset::GroupprintsGroup, but the user typedgroup. ImplementDisplay(or useValueEnum::to_possible_value().unwrap().get_name()) for a log line that matches the CLI surface.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/commands/compare/bams.rs` around lines 958 - 964, The log prints the preset using Debug ("{:?}") which shows `Group` instead of the CLI-cased `group`; change the log to emit the CLI name by either implementing Display for CommandPreset and using "{}" (implement Display for the CommandPreset enum) or by calling the clap ValueEnum helper on the existing preset variable (e.g., use preset.to_possible_value().unwrap().get_name() or preset.as_str() if available) in the info! call so the message matches the CLI surface; update the info! invocation that references preset to use the chosen method and preserve the rest of the interpolation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/lib/commands/compare/bams.rs`:
- Around line 210-216: Update the doc comment for the CompareMode `mode` field
to remove stale references to "clip/downsample" and list the current command
presets supported by the enum (extract/zipper/sort/correct/dedup/filter); locate
the doc above the `pub mode: Option<CompareMode>` field and replace the old
preset examples so the comment aligns exactly with the current CompareMode
variants and intended usage.
- Around line 2015-2043: Replace the manual loops in the tests
preset_defaults_content_stages_map_to_content_no_ignore_order and
preset_defaults_grouping_stages_map_to_grouping_with_ignore_order with rstest
parameterized cases: annotate each test function with #[rstest] and add a
#[case] for each CommandPreset variant used (e.g., CommandPreset::Extract,
::Zipper, ::Sort, ::Correct, ::Dedup, ::Filter for the content test and
CommandPreset::Group, ::Simplex, ::Duplex, ::Codec for the grouping test),
change the function signature to accept a single parameter (#[case] stage:
CommandPreset), and keep the assertions that stage.defaults() returns the
expected CompareMode and ignore boolean; this makes each variant an independent
rstest case and removes the for-loop.
- Around line 950-965: The validation currently bails on ignore_order regardless
of whether it was set from a preset or explicitly; update the logic around
effective_settings()/the subsequent check so that if ignore_order is true but it
was inherited from a preset (i.e. self.command.is_some() &&
!self.ignore_order.is_some() && self.mode.is_some()), you either clear
ignore_order when resolved mode != CompareMode::Grouping (set
ignore_order=false) or change the error message to mention the preset origin;
locate the check using effective_settings(), CompareMode::Grouping and the
fields self.command, self.mode, self.ignore_order and implement the chosen fix
so explicit --mode overrides do not produce a misleading "--ignore-order" error.
---
Nitpick comments:
In `@src/lib/commands/compare/bams.rs`:
- Around line 958-964: The log prints the preset using Debug ("{:?}") which
shows `Group` instead of the CLI-cased `group`; change the log to emit the CLI
name by either implementing Display for CommandPreset and using "{}" (implement
Display for the CommandPreset enum) or by calling the clap ValueEnum helper on
the existing preset variable (e.g., use
preset.to_possible_value().unwrap().get_name() or preset.as_str() if available)
in the info! call so the message matches the CLI surface; update the info!
invocation that references preset to use the chosen method and preserve the rest
of the interpolation.
🪄 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: 80306360-34d7-491b-9903-b70704cd01e3
📒 Files selected for processing (1)
src/lib/commands/compare/bams.rs
|
Only repository collaborators, contributors, or members can run CodeRabbit commands. |
Reintroduce the `--command <stage>` preset on `fgumi compare bams`, which was originally added in #276 and unintentionally removed in #296 when that breaking refactor synced compare/bams.rs from a pre-#276 backup branch. The preset sets canonical `--mode` and `--ignore-order` defaults for comparing BAM output from a specific pipeline stage, and is especially useful for cross-tool comparisons (e.g. fgumi vs. fgbio) where MI values or record order may legitimately differ. Adapted to the current `--mode {full,content,grouping}` + `--ignore-order` API (post-#276), with defaults tuned for benchmarks' cross-tool needs: extract/zipper/sort/correct/dedup/filter → content, false group/simplex/duplex/codec → grouping, true Explicit `--mode` or `--ignore-order` flags override the preset. The `mode` and `ignore_order` struct fields become `Option<T>` so the resolver can distinguish "unset" from "explicit default". - `mode`: logs preset usage when `--command` is provided - `effective_settings()` resolves explicit > preset > built-in default - 6 new unit tests cover the resolver and preset defaults; all 2458 existing tests still pass Restores the contract that fgumi-benchmarks `compare_bams_batch` relies on (workflow/rules/benchmark.smk `get_compare_args()`), which currently fails immediately with "error: unexpected argument '--command' found".
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
d0c48df to
853542e
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
This branch was previously deployed
1 participant
Summary
Reintroduce the
--command <stage>preset onfgumi compare bams, which was originally added in #276 and unintentionally removed in #296 when that breaking refactor syncedsrc/lib/commands/compare/bams.rsfrom a pre-#276 backup branch. Thecompare/bams.rsedits in #296 were absorbed into an "amends to include" bullet alongside the larger bam_codec/simulate scope and the regression slipped through review.Why this matters
Downstream
fgumi-benchmarksrelies on this flag for cross-tool equivalency checks between fgumi and fgbio outputs (workflow/rules/benchmark.smk→get_compare_args()). At the current fgumi main, everycompare_bams_batchjob that hits a stage in the preset set fails immediately with:Other batches (e.g.
group.identity) silently run with--mode fulland fail on expected cross-tool ordering differences, because the preset that would have set--mode grouping --ignore-orderwas also lost.What the preset does
--command <stage>applies canonical--modeand--ignore-orderdefaults. Adapted to the post-#276--mode {full,content,grouping}+--ignore-ordersurface:--mode--ignore-order--threadsExplicit
--modeor--ignore-orderflags always override the preset, e.g.:Implementation
CommandPresetenum reintroduced with a.defaults() -> (CompareMode, bool)method.modeandignore_orderstruct fields changed toOption<T>so the resolver can distinguish "unset" from "explicit value". This is internal — the CLI surface still has--mode/--ignore-orderwith no default, and the old default-full/ default-falsebehavior is preserved byeffective_settings().effective_settings(&self) -> (CompareMode, bool)resolves precedence: explicit flag >--commandpreset default > built-in default (full,false).execute()logsUsing --command <preset> preset: mode=..., ignore-order=... (with explicit overrides)when--commandis in effect.COMMAND PRESETSsection replacing theRECOMMENDED SETTINGS BY COMMANDtable (same information, self-documenting now that the preset is back).Test plan
cargo build --release --features compare,simulate— builds cleancargo ci-test— 2458 tests pass, 23 skipped; no regressionscargo ci-fmt— cleancargo ci-lint— cleansrc/lib/commands/compare/bams.rs::tests:preset_defaults_content_stages_map_to_content_no_ignore_orderpreset_defaults_grouping_stages_map_to_grouping_with_ignore_ordereffective_settings_fall_back_to_built_in_defaultseffective_settings_use_preset_when_only_command_giveneffective_settings_explicit_mode_overrides_preseteffective_settings_explicit_ignore_order_overrides_presetfgumi compare bams --help— verified--commandappears and preset table renders correctlytest_compare_bamsintegration tests still passOut of scope
--mode/--ignore-ordersemantics or any comparison logic — pure CLI / preset-resolution change.clip,downsample,reviewpresets (they were not in feat(compare): add --command preset for per-stage compare defaults #276's original enum and are not used by the benchmarks workflow).