Skip to content

fix(runall): wire --methylation-mode/--ref into the fused filter stage #944

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
nh13 merged 1 commit into from
Sep 11, 2026
Merged

fix(runall): wire --methylation-mode/--ref into the fused filter stage #944

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
210 changes: 190 additions & 20 deletions src/lib/commands/runall.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -937,17 +937,18 @@ impl RunAll {
})
}

/// Returns the reference FASTA to thread into the consensus caller's
/// `#[arg(skip)]` `reference` slot, gated on `--methylation-mode` being
/// set: `Some(self.reference.clone())` when methylation mode is
/// requested, `None` otherwise (even if `--ref` was supplied — e.g. to
/// feed an upstream align stage in the same chain).
/// Returns the reference FASTA to thread into a methylation-consuming
/// stage's `reference` slot, gated on `--methylation-mode` being set:
/// `Some(self.reference.clone())` when methylation mode is requested,
/// `None` otherwise (even if `--ref` was supplied — e.g. to feed an
/// upstream align stage in the same chain).
///
/// Called by [`Self::build_stage_options_bag`] to populate the
/// `#[arg(skip)]` `reference` slot on the Simplex/Duplex consensus
/// options structs.
/// `#[arg(skip)]` `reference` slot on the Simplex/Duplex consensus options
/// structs, and to override the Filter stage's `reference` when methylation
/// filtering is requested.
#[must_use]
pub(crate) fn consensus_reference(&self) -> Option<PathBuf> {
pub(crate) fn methylation_reference(&self) -> Option<PathBuf> {
if self.methylation_mode.is_some() { self.reference.clone() } else { None }
}

Expand Down Expand Up @@ -1356,7 +1357,7 @@ impl RunAll {
opts.read_group.clone_from(&self.read_group);
opts.methylation_mode =
crate::commands::common::resolve_methylation_mode(self.methylation_mode);
opts.reference = self.consensus_reference();
opts.reference = self.methylation_reference();
bag.simplex = Some(opts);
}

Expand All @@ -1374,7 +1375,7 @@ impl RunAll {
opts.read_group.clone_from(&self.read_group);
opts.methylation_mode =
crate::commands::common::resolve_methylation_mode(self.methylation_mode);
opts.reference = self.consensus_reference();
opts.reference = self.methylation_reference();
bag.duplex = Some(opts);
}

Expand Down Expand Up @@ -1440,12 +1441,30 @@ impl RunAll {
}

Stage::Filter => {
// The standalone filter chain builder reads `rejects` and
// `stats` straight off the bag's FilterOptions, so
// `--filter::rejects` / `--filter::stats` flow through
// unchanged. No cross-stage rewiring is needed: filter is
// always terminal in a runall chain.
let filter_opts = self.filter_opts.clone().validate()?;
// `--filter::rejects` / `--filter::stats` flow through the
// projected FilterOptions unchanged. Filter is *also* a
// methylation-consuming stage: `--filter::min-conversion-fraction`
// reads the resolved `methylation_mode` (and the `reference`),
// and `--filter::require-strand-methylation-agreement` reads the
// `reference`; so thread the top-level cross-stage
// `--methylation-mode` / `--ref` into the projected options
// exactly as the Simplex/Duplex arms do. This must happen
// before the chain builder runs `filter.validate_parameters()`
// (builder.rs add_filter); otherwise the fused stage sees
// `MethylationMode::Disabled` and rejects a legitimately-set
// `--methylation-mode`. (The up-front guard above only lets
// `--methylation-mode` reach here when a consuming option is
// set, so this injection is never a dead write.)
let mut filter_opts = self.filter_opts.clone().validate()?;
filter_opts.methylation_mode =
crate::commands::common::resolve_methylation_mode(self.methylation_mode);
// `methylation_reference()` yields `Some(--ref)` only when
// methylation is requested; leave the projected `--filter::ref`
// untouched otherwise (it still feeds the mapped-read NM/UQ/MD
// path when a user supplies it directly).
if let Some(reference) = self.methylation_reference() {
filter_opts.reference = Some(reference);
}
bag.filter = Some(filter_opts);
}

Expand Down Expand Up @@ -1534,7 +1553,7 @@ impl Command for RunAll {
let chain_reaches_consensus = stages.iter().any(|s| s.is_consensus());
let chain_includes_align = stages.contains(&Stage::Align);
// `--methylation-mode` on a simplex/duplex chain requires `--ref`: the
// consensus stage consumes the FASTA (via `consensus_reference()`), and
// consensus stage consumes the FASTA (via `methylation_reference()`), and
// the standalone contract (`--methylation-mode` doc, "requires --ref")
// demands it. Without this guard a non-align `group → simplex` run with
// `--methylation-mode` but no `--ref` slips through (the dead-consensus
Expand All @@ -1553,10 +1572,21 @@ impl Command for RunAll {
{
bail!("--methylation-mode requires --ref to be set");
}
if self.methylation_mode.is_some() && !chain_reaches_consensus && !chain_includes_align {
// Filter consumes `--methylation-mode` ONLY through
// `--filter::min-conversion-fraction` (the one filter option whose check
// reads the resolved `methylation_mode`; `--require-strand-methylation-agreement`
// and `--min-methylation-depth` use the reference/methylation tags but never
// the mode). So a filter chain keeps the flag live only when that option is
// set — otherwise `--methylation-mode` would be silently inert on the filter
// stage, which is exactly the silent-ignore this guard rejects.
let filter_consumes_methylation = stages.contains(&Stage::Filter)
&& self.filter_opts.filter_min_conversion_fraction.is_some();
let chain_consumes_methylation = chain_reaches_consensus || filter_consumes_methylation;
if self.methylation_mode.is_some() && !chain_consumes_methylation && !chain_includes_align {
bail!(
"--methylation-mode is only consumed by the consensus stage; \
it is dead on a runall chain that stops before consensus"
"--methylation-mode is consumed only by the consensus stages and by \
filter's --min-conversion-fraction; it is dead on a runall chain that \
reaches neither"
);
}

Expand Down Expand Up @@ -1782,6 +1812,65 @@ mod execute_tests {
assert!(e.contains("--ref requires --methylation-mode"), "got: {e}");
}

#[test]
fn filter_methylation_with_conversion_fraction_passes_the_dead_flag_guard() {
// A filter self-pair with `--methylation-mode` AND a methylation-consuming
// option (`--filter::min-conversion-fraction`, the one filter option that
// reads the resolved mode) must NOT be rejected by the dead-flag guard —
// the flag is genuinely live. The run still errors (the input path does not
// exist), but it must get past the guard (audit A1 / HIGH). The needle is a
// substring of the CURRENT guard message, so reverting the guard fix (or the
// min-conversion-fraction narrowing) makes this assertion fail.
let e = run(&[
"--start-from",
"filter",
"--stop-after",
"filter",
"-i",
"in.bam",
"-o",
"o.bam",
"--filter::min-reads",
"1",
"--filter::min-conversion-fraction",
"0.5",
"--methylation-mode",
"em-seq",
"--ref",
"ref.fa",
])
.unwrap_err()
.to_string();
assert!(!e.contains("dead on a runall chain"), "guard wrongly fired: {e}");
assert!(!e.contains("requires --methylation-mode to be set"), "premature reject: {e}");
}

#[test]
fn bare_filter_methylation_is_rejected_as_inert() {
// Conversely, `--methylation-mode` on a filter chain with NO
// methylation-consuming option set is silently inert (filter reads the mode
// only via `--min-conversion-fraction`), so the guard MUST reject it up front
// rather than accept a no-op flag. Locks the guard against being widened too
// far — the exact regression the gauntlet caught in the first cut of this fix.
let e = run(&[
"--start-from",
"filter",
"--stop-after",
"filter",
"-i",
"in.bam",
"-o",
"o.bam",
"--filter::min-reads",
"1",
"--methylation-mode",
"em-seq",
])
.unwrap_err()
.to_string();
assert!(e.contains("dead on a runall chain"), "guard should reject the inert flag: {e}");
}

#[test]
fn rejects_stop_after_align_at_clap_layer() {
// StopAfter has no `align` variant → clap rejects it before execute().
Expand Down Expand Up @@ -2011,6 +2100,87 @@ mod bag_tests {
assert_eq!(g.effective_edits, 1);
}

#[test]
fn filter_receives_top_level_methylation_and_ref() {
// A fused chain that reaches filter must thread the top-level
// --methylation-mode / --ref into the filter stage exactly as the
// Simplex/Duplex arms do; otherwise the chain builder's
// `filter.validate_parameters()` sees MethylationMode::Disabled and
// rejects a legitimately-set --methylation-mode (audit A1 / HIGH).
let r = parse(&[
"--start-from",
"filter",
"--stop-after",
"filter",
"-i",
"in.bam",
"-o",
"out.bam",
"--filter::min-reads",
"1",
"--filter::min-conversion-fraction",
"0.5",
"--methylation-mode",
"em-seq",
"--ref",
"ref.fa",
]);
let f = r.build_stage_options_bag(&[Stage::Filter]).unwrap().filter.unwrap();
assert_eq!(f.methylation_mode, fgumi_consensus::MethylationMode::EmSeq);
assert_eq!(f.reference.as_deref(), Some(std::path::Path::new("ref.fa")));
}

#[test]
fn filter_without_methylation_leaves_mode_disabled() {
// Without --methylation-mode the injected mode stays Disabled and the
// top-level --ref is not forced onto the filter stage (methylation_reference
// returns None), so a non-methylation filter chain is unaffected.
let r = parse(&[
"--start-from",
"filter",
"--stop-after",
"filter",
"-i",
"in.bam",
"-o",
"out.bam",
"--filter::min-reads",
"1",
]);
let f = r.build_stage_options_bag(&[Stage::Filter]).unwrap().filter.unwrap();
assert_eq!(f.methylation_mode, fgumi_consensus::MethylationMode::Disabled);
assert_eq!(f.reference, None);
}

#[test]
fn filter_ref_flag_supplies_reference_when_no_top_level_ref() {
// When methylation is requested but only the projected `--filter::ref`
// is given (no top-level `--ref`), `methylation_reference()` returns None,
// so the injection leaves the projected reference in place rather than
// clobbering it — the alternative reference source stays usable.
let r = parse(&[
"--start-from",
"filter",
"--stop-after",
"filter",
"-i",
"in.bam",
"-o",
"out.bam",
"--filter::min-reads",
"1",
"--filter::min-conversion-fraction",
"0.5",
"--filter::ref",
"filter_ref.fa",
"--methylation-mode",
"em-seq",
]);
let f = r.build_stage_options_bag(&[Stage::Filter]).unwrap().filter.unwrap();
assert_eq!(f.methylation_mode, fgumi_consensus::MethylationMode::EmSeq);
assert_eq!(f.reference.as_deref(), Some(std::path::Path::new("filter_ref.fa")));
}

#[test]
fn fused_sort_max_memory_auto_is_overridden_to_fixed() {
let r = parse(&[
Expand Down
90 changes: 89 additions & 1 deletion tests/integration/test_runall_command.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -454,7 +454,7 @@ fn simplex_self_pair_matches_standalone_simplex() {

/// Spec §7: `--methylation-mode` (+ `--ref`) must actually reach the consensus
/// stage's `#[arg(skip)]` `methylation_mode`/`reference` slots (wired via
/// `resolve_methylation_mode`/`consensus_reference` in
/// `resolve_methylation_mode`/`methylation_reference` in
/// `build_stage_options_bag`), not just be accepted and silently dropped.
/// Compares the fused `consensus(simplex)` self-pair against the standalone
/// `fgumi simplex --methylation-mode em-seq --ref ...` oracle — record parity
Expand Down Expand Up @@ -867,6 +867,94 @@ fn group_to_simplex_to_filter_matches_staged_chain() {
assert_bam_headers_equivalent_ignoring_pg(&runall_out, &staged_out);
}

/// Audit A1: the fused filter stage must thread the top-level `--methylation-mode`
/// / `--ref` into filter's methylation-aware options — `--filter::min-conversion-fraction`
/// requires BOTH to be set. Before the wiring fix, the fused stage saw
/// `MethylationMode::Disabled` and rejected the legitimately-set flag ("requires
/// --methylation-mode to be set"), so the methylation filters were unreachable
/// through runall. Compares the fused consensus(simplex)->filter chain against the
/// staged equivalent where standalone simplex and filter each receive
/// `--methylation-mode em-seq --ref`; `--min-conversion-fraction 0.0` exercises the
/// threaded flags (validation requires them) while keeping the output non-empty.
/// Reuses the same `grouped_bam` + `create_test_reference` fixture as
/// `simplex_self_pair_with_methylation_mode_matches_standalone`, which is known to
/// produce methylation-tagged consensus records.
#[cfg(feature = "consensus")]
#[test]
fn consensus_to_filter_with_methylation_matches_staged_chain() {
let tmp = TempDir::new().unwrap();
let fixture = grouped_bam(tmp.path(), "identity", "filter_methylation");
let reference = create_test_reference(tmp.path());
let runall_out = tmp.path().join("runall.bam");
let staged_simplex = tmp.path().join("staged_simplex.bam");
let staged_out = tmp.path().join("staged.bam");

run_ok(
[
"runall",
"--start-from",
"consensus",
"--stop-after",
"filter",
"--consensus",
"simplex",
"-i",
p(&fixture),
"-o",
p(&runall_out),
"--simplex::min-reads",
"1",
"--filter::min-reads",
"1",
"--filter::min-conversion-fraction",
"0.0",
"--methylation-mode",
"em-seq",
"--ref",
p(&reference),
],
"runall consensus(simplex)->filter+methylation",
);

run_ok(
[
"simplex",
"-i",
p(&fixture),
"-o",
p(&staged_simplex),
"--min-reads",
"1",
"--methylation-mode",
"em-seq",
"--ref",
p(&reference),
],
"staged simplex+methylation",
);
run_ok(
[
"filter",
"-i",
p(&staged_simplex),
"-o",
p(&staged_out),
"--min-reads",
"1",
"--min-conversion-fraction",
"0.0",
"--methylation-mode",
"em-seq",
"--ref",
p(&reference),
],
"staged filter+methylation",
);

assert_bams_record_equivalent_nonempty(&runall_out, &staged_out);
assert_bam_headers_equivalent_ignoring_pg(&runall_out, &staged_out);
}

// ══════════════════════════ Extract→Correct (no aligner) ══════════════════════════

/// A small paired gzip FASTQ pair (`r1.fq.gz`, `r2.fq.gz`) with a 4 bp UMI on
Expand Down
Loading