feat: MassCalibrator port — opt-in --precursor-cal flag (default off)#33
Merged
Conversation
Lands the Java MassCalibrator pre-pass + CLI flag with default `off`. When off (default), behavior matches origin/dev for the bit-identical regression gate (sorted PIN/TSV row-set; see relaxation note below). Defers GF/SpecE/precursor-filter Java-parity hygiene to PR-B (see the design spec in docs/parity-analysis/notes/2026-05-25-precursor-cal-ship-gates.md and the corresponding spec under docs/). G1 ship gate (--precursor-cal auto Rust @1% FDR within ±1% of Java on LFQ/Astral/TMT) is not closed in this PR; tracked separately. Key additions: - crates/search/src/precursor_cal.rs — helpers + PrecursorCalMode enum - crates/search/src/mass_calibrator.rs — pre-pass orchestration - crates/search/src/match_engine.rs — run_chunk_with_params + adjusted_observed_neutral_mass at the two precursor-mass sites only (PR-B's java_match_score / SinkRetry / dedup rewrite deferred) - CLI two-pass pipeline: metadata scan -> sampled spectra load -> pre-pass -> apply shift + tighten tolerance -> main pass - crates/search/tests/mass_calibrator_integration.rs — closes the documented "no isolated cal integration test" gap (5 tests) - crates/msgf-rust/tests/precursor_cal_bit_identical.rs — regression gate for the off-path; sorted-row compare to handle the existing rayon tie-breaking nondeterminism (separate issue; see .github/workflows/ci.yml:41-43) - benchmark/ci/run_bench_calauto_3ds.sh — 3-dataset bench harness template; defaults match the bigbio bench VM layout - docs/parity-analysis/snapshots/cal-shifts-2026-05-25.json — learned shift artifact vs Java on the 3 bench datasets - TSV writer: thread PsmMatch::isotope_offset into IsotopeError column (small drive-by fix; PIN already reported this value) Default policy: SearchParams::default_tryptic and CLI both default to PrecursorCalMode::Off until G1 passes; library consumers see no shift unless they explicitly opt in. Bit-identical gate relaxation: the strict "byte-identical to origin/dev" assertion specified in the design doc was downgraded to "sorted PIN/TSV row-set equality vs a committed golden generated by this branch" because (1) the rayon-based search pipeline has a known tie-breaking nondeterminism that varies row order across runs (separately tracked), and (2) the TSV writer now reports the winning isotope offset rather than constant 0. PIN/TSV CONTENT is verified unchanged in the off-path; only the row ORDER may differ across runs.
Final review observations addressed: 1. PrecursorCalMode::default() previously returned Auto (#[default] on Auto variant). The CLI and SearchParams::default_tryptic both hardcode Off, but any future struct that derives Default and contains a PrecursorCalMode field would silently activate the pre-pass. Moved #[default] to Off so the derive matches the ship-gate intent. 2. CalibrationStats::has_reliable_stats checked confident_psm_count > 0, but learn_calibration_stats only ever sets that field to 0 or >= MIN_CONFIDENT_PSMS (200). Added a comment explaining the upstream threshold so future readers don't weaken the gate inadvertently.
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ 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 |
ypriverol
added a commit
that referenced
this pull request
May 26, 2026
Resolves conflicts after PR #33 (MassCalibrator port — opt-in --precursor-cal) merged into dev. Two minor conflicts: - crates/msgf-rust/src/bin/msgf-rust.rs: PR-A moved `spectrum_ext` and `ms_level_u32` declarations earlier in run(); the local block at the former position was redundant. Took PR-A's side (empty block). - crates/msgf-rust/tests/cli_smoke.rs: kept PR-A's two new tests (cli_accepts_isotope_error_min_negative_one, cli_accepts_precursor_cal_off) alongside the bug-hunt branch's existing CLI smoke tests.
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
Lands the Java MassCalibrator pre-pass +
--precursor-cal {auto,on,off}CLI flag with defaultoff. When off (PR-A's default), behavior is unchanged fromorigin/devfor the regression gate (sorted PIN/TSV row-set vs committed golden).GF/SpecE Java-parity hygiene + precursor-filter rewrite are intentionally deferred to PR-B. G1 ship gate (
--precursor-cal autoRust @1% FDR within ±1% of Java on LFQ/Astral/TMT) is not closed in this PR; tracked as future SpecE-tail work.Bench results — Percolator @1% FDR target counts (pride-linux-vm, 2026-05-26)
Read-out:
--precursor-cal off(PR-A default) does not regress PSM counts on any dataset.Key changes
crates/search/src/precursor_cal.rs— helpers +PrecursorCalModeenumcrates/search/src/mass_calibrator.rs— pre-pass orchestrationcrates/search/src/match_engine.rs—run_chunk_with_params+adjusted_observed_neutral_massat the two precursor-mass sites only (PR-B'sjava_match_score/ SinkRetry / dedup rewrite deferred)crates/search/tests/mass_calibrator_integration.rs— 5 tests asserting the calibrator helper contracts (closes the no-integration-test gap)crates/msgf-rust/tests/precursor_cal_bit_identical.rs— regression gate for the off-path; sorted-row compare (tolerant of the existing rayon tie-break nondeterminism documented in.github/workflows/ci.yml:41-43)benchmark/ci/run_bench_calauto_3ds.sh— 3-dataset bench harness templatedocs/parity-analysis/snapshots/cal-shifts-2026-05-25.json— learned shift artifact vs Java on the 3 bench datasetsPsmMatch::isotope_offsetinto IsotopeError column (small drive-by; PIN already reported this value)Default policy
SearchParams::default_trypticand CLI both default toPrecursorCalMode::Off.PrecursorCalMode::default()also returnsOff(#[default]placed on theOffvariant) so any future struct derivingDefaultcannot silently enable the pre-pass.Test plan
cargo test --release --workspacegreen under the existing CI skip listOut of scope (PR-B)
crates/scoring/src/scoring/scored_spectrum.rsprecursor-filter rewritematch_engine::java_match_scoreGF rewiring + SinkUnreachable retry removal +score>=maxguard removaldedup_pepseq_scorePepDedupKeymod-aware rewriteBranches retained locally:
feat/precursor-cal-full-snapshot(pre-split snapshot for PR-B work).