Skip to content

Treat non-ppm precursor tolerance as 10 ppm default for MS1 envelope - #41

Merged
brendanx67 merged 3 commits into
mainfrom
fix/non-ppm-precursor-tolerance-ms1-default
May 26, 2026
Merged

Treat non-ppm precursor tolerance as 10 ppm default for MS1 envelope#41
brendanx67 merged 3 commits into
mainfrom
fix/non-ppm-precursor-tolerance-ms1-default

Conversation

@brendanx67

Copy link
Copy Markdown
Collaborator

Summary

run_coelution_calibration_scoring (both pass 1 and pass 2 in
pipeline.rs) was passing config.precursor_tolerance.tolerance
into the MS1 isotope-envelope extractor regardless of the unit. For
unit-resolution config (e.g. Stellar: tolerance=1.0, unit=Mz),
this was treating 1.0 Da as 1.0 ppm, producing a ~0.5 mDa
window at 500 m/z — effectively zero. envelope.has_m0() returned
false on ~99.8% of matches and only 18 of 7361 calibration-passing
entries contributed to ms1_calibration on Stellar Single, vs 193
on C# (10x undercount).

Fix

New helper ms1_envelope_tolerance_ppm in pipeline.rs:

fn ms1_envelope_tolerance_ppm(config: &OspreyConfig) -> f64 {
    match config.precursor_tolerance.unit {
        ToleranceUnit::Ppm => config.precursor_tolerance.tolerance,
        _ => 10.0,
    }
}

Wired into both pass-1 and pass-2 run_coelution_calibration_scoring
call sites. Mirrors the C# OspreySharp behavior in
PerFileScoringTask.ScoreCalibrationEntry (unit == Ppm ? tolerance : 10.0).

Cross-impl effect

On Stellar Single calibration.json, the
ms1_calibration.{count, mean, median, sd, adjusted_tolerance} drift
disappears from the cross-impl diff. It was the dominant
CAL_JSON divergence (diff 1.75e+2 at
ms1_calibration.count). Remaining cal_json drift is
rt_calibration.model_params.abs_residuals[i] sort-order swaps on
a handful of indices where the underlying values are bit-equal but
sorted slightly differently cross-impl (addressed in a sibling PR).

Production impact

For runs with ppm-unit precursor_tolerance (most HRAM workflows),
behavior is unchanged — the helper returns the configured value.

For runs with non-ppm precursor_tolerance (unit-resolution,
mass-window, etc.) the MS1 envelope tolerance now defaults to 10 ppm
instead of silently passing through the absolute value as if it
were ppm. This is the correct behavior — the envelope extractor
requires a ppm window — and matches the OspreySharp port.
Calibration counts will increase for these configurations
(typically by ~10×), which is the bug fix.

Detection

Found via OSPREY_DIAG_MS1 instrumentation showing tol_ppm=1.0
and peaks_in_m0_window=0 on all sampled calibration entries.

Testing

  • cargo fmt --check
  • cargo clippy --all-targets --all-features -- -D warnings
  • cargo test
  • Cross-impl: Stellar Single calibration.json ms1_calibration
    block now matches C# OspreySharp; previously the dominant per-
    file calibration divergence.

The MS1 isotope envelope extraction at calibration time (batch.rs:2708
in run_coelution_calibration_scoring) takes a tolerance_ppm argument
and uses it for FindPeakPpm-style matching of the M+0 peak. Rust was
passing config.precursor_tolerance.tolerance regardless of unit. For
unit-resolution data (Stellar config: tolerance=1.0, unit=Mz), this
treated 1.0 Da as 1.0 ppm and produced a ~0.5 mDa window at 500 m/z -
effectively zero. As a result envelope.has_m0() returned false on
~99.8% of matches and only 18 of 7361 calibration-passing entries
contributed to ms1_calibration on Stellar Single, vs 193 on C#
(10x undercount).

C# OspreySharp's PerFileScoringTask.ScoreCalibrationEntry handles the
same case via `unit == Ppm ? tolerance : 10.0`. This adds a Rust
helper `ms1_envelope_tolerance_ppm` with identical behavior and wires
it into both pass-1 and pass-2 run_coelution_calibration_scoring
callsites in pipeline.rs.

Cross-impl effect on Stellar Single calibration.json:
ms1_calibration.{count, mean, median, sd, adjusted_tolerance} drift
disappears from the diff (was the dominant CAL_JSON divergence with
diff 1.75e+2 at ms1_calibration.count). Remaining cal_json drift is
rt_calibration.model_params.abs_residuals[i] sort-order swaps on a
handful of indices, where the underlying values are bit-equal but
sorted slightly differently cross-impl.

Found via OSPREY_DIAG_MS1 instrumentation showing tol_ppm=1.0 and
peaks_in_m0_window=0 on all sampled calibration entries.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Fixes MS1 isotope-envelope extraction in calibration scoring when precursor_tolerance is configured in non-ppm units (e.g., unit-resolution workflows). The pipeline previously passed the raw tolerance value as if it were ppm, effectively collapsing the MS1 envelope window and causing most envelopes to fail has_m0().

Changes:

  • Added a ms1_envelope_tolerance_ppm helper that converts precursor tolerance to a ppm value usable by the MS1 envelope extractor (ppm passthrough; non-ppm defaults to 10 ppm).
  • Updated both pass-1 and pass-2 run_coelution_calibration_scoring call sites to use the helper instead of config.precursor_tolerance.tolerance.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread crates/osprey/src/pipeline.rs Outdated
Comment thread crates/osprey/src/pipeline.rs
…e helper

* Comment now correctly states the 1-ppm-treated 1 Da window at 500 m/z
  is 500 * 1e-6 = 0.5 mDa = 5e-4 Th (was off by 1000x as "0.0005 mDa")
* Added two unit tests pinning ms1_envelope_tolerance_ppm behavior:
  Ppm input passes through; any Mz input maps to the 10 ppm default
  regardless of the configured numeric value
…doc nit

* Tests now use FragmentToleranceConfig::hram / ::unit_resolution
  constructors (catches future drift in defaults)
* Added a third test pinning that Ppm(0.0) returns 0.0 -- the 10 ppm
  fallback fires only on the Mz branch and must not silently rescue
  a misconfigured zero on the Ppm side
* Doc note that OspreyConfig::precursor_tolerance reuses the
  FragmentToleranceConfig struct (historical name)
@brendanx67
brendanx67 merged commit fae0f7a into main May 26, 2026
3 checks passed
@brendanx67
brendanx67 deleted the fix/non-ppm-precursor-tolerance-ms1-default branch May 26, 2026 01:51
brendanx67 added a commit that referenced this pull request May 26, 2026
…mp_ms2_cal_errors diagnostic

train_and_score_calibration sorts all_matches by discriminant_score-
descending as a side effect of LDA scoring. The downstream
mz_qc_data accumulator (Welford running sum, in mass.rs) and the
LOESS RT calibration both iterate `passing_targets` / `refined_passing`
in that score order, while C#'s OspreySharp matchArray stays in
(base_id, entry_id) order. Different iteration orders feed the
running sum different sequences, leaving a 1-ULP residual in MS2
calibration mean even when the input set is bit-identical.

Sort the matches by (base_id, entry_id) right before the
accumulator loop in both pass 1 and pass 2 to match the C# port
byte-for-byte. The shadowing rebind in pass 2 keeps the variable
name `refined_passing` so subsequent code reads as before.

Adds OSPREY_DUMP_MS2_CAL_ERRORS (in osprey-scoring/src/diagnostics.rs
with the matching writer + invocations at both passes in pipeline.rs),
which writes rust_ms2_cal_errors.txt — one row per per-fragment
error in the same order the calibration mean sees them. Pair with
the OspreySharp WriteMs2CalErrorsDump and ai/.tmp/diff_ms2_cal_errors.py
to bisect future MS2 calibration accumulator regressions.

Welford running mean itself is already on main via PR #39
(Bucket 2 bit-parity tweaks); this commit re-applies the same
mass.rs hunk for completeness via cherry-pick from the original
3aca00c commit, which git treats as a no-op against the already-
merged code. The behavioral content of this commit is the sort
fix and the new diagnostic.

Verified: Stellar + Astral 3-file end-to-end cross-impl PASS at
1e-9 against OspreySharp on an aggregated branch combining this
fix with the other open PRs (#40, #41, #42, #43, #131-equivalent).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants