Clamp experiment q-value to each entry's best run q-value (cross-impl parity with pwiz#4390) - #49
Merged
Conversation
Experiment-level FDR competes each precursor's single best observation against a de-duplicated (thinner) decoy null, so the raw experiment q-value can fall below every per-run q-value. That lets a precursor pass experiment-level FDR with no single run passing run-level FDR, producing reported peptides with no run-level ID (the blib ID-line artifact) and an anti-conservative experiment-wide FDP calibration. Add clamp_experiment_q_to_best_run, which floors each entry's experiment_precursor_qvalue / experiment_peptide_qvalue up to that entry's best (min-over-runs) combined run q-value (FdrLevel::Both = max(run_precursor, run_peptide)). The precursor floor keys on the full entry_id (decoy high bit included) and the peptide floor on (modified_sequence, is_decoy) - both target/decoy-specific, so a target never inherits its paired decoy's good run. An empty modified_sequence (a stub loaded without the column) has no peptide identity and is skipped. The clamp runs at the end of run_percolator_fdr for both the direct and streaming paths (covering first-pass and the reconciliation-aware second pass), and again unconditionally on the final post-reconciliation pool right before the blib output gate, since Stage-6 reconciliation resets the run q-value of moved / gap-fill peaks after the in-pass clamp. The two HPC --join-at-pass=2 recompute paths are left unclamped for now, each marked with a PARITY NOTE for a follow-up. This mirrors PercolatorEngine.ClampExperimentQToBestRun and its two call sites in the C# implementation, for cross-impl parity.
brendanx67
added a commit
to ProteoWizard/pwiz-ai
that referenced
this pull request
Jul 8, 2026
…klog * Q-value filtering TODO: Brendan accepted (model-diagnostics libdecoy re-check); Copilot addressed (94f8e80bc3); Rust parity PR maccoss/osprey#49 opened. * Added backlog TODO for the gendecoy salvage-or-deadend decision.
Merged
2 tasks
maccoss
added a commit
that referenced
this pull request
Jul 13, 2026
…ned intensity features (#55) * Carry the join-wide first-pass base_id set in reconciliation.json (v3) The per-file <stem>.reconciliation.json boundary file now carries first_pass_base_ids, the join-wide set of base_ids that survived first-pass compaction (format_version 2 -> 3), written sorted ascending from the first-pass passing set the pipeline already computes before compaction. On the OspreySharp side this set lets a per-file HPC rescore worker compact to exactly the set the in-memory straight-through pipeline used, instead of recomputing a per-file subset that drops cross-file entries. This commit is the Rust half: it keeps the reconciliation.json format byte-identical across impls so the parity tests stay copy-free. Wiring the Rust per-file worker to consume the set (the analogous HPC self-consistency fix) is a follow-up. * first_pass_base_ids is required in v3 (no serde default): v2 envelopes are rejected at parse time rather than a worker silently recomputing a divergent subset. Mirrors the OspreySharp hard-fail. Rust half of a cross-impl change with OspreySharp (ProteoWizard/pwiz #4353). Verified: the round-trip byte-parity hook (OSPREY_CROSS_IMPL_RECONCILIATION_OUT) produces a reconciliation.json byte-identical to OspreySharp's IOTest.TestReconciliationFileRoundTrip. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * Clamp experiment q-value to each entry's best run q-value (#49) Experiment-level FDR competes each precursor's single best observation against a de-duplicated (thinner) decoy null, so the raw experiment q-value can fall below every per-run q-value. That lets a precursor pass experiment-level FDR with no single run passing run-level FDR, producing reported peptides with no run-level ID (the blib ID-line artifact) and an anti-conservative experiment-wide FDP calibration. Add clamp_experiment_q_to_best_run, which floors each entry's experiment_precursor_qvalue / experiment_peptide_qvalue up to that entry's best (min-over-runs) combined run q-value (FdrLevel::Both = max(run_precursor, run_peptide)). The precursor floor keys on the full entry_id (decoy high bit included) and the peptide floor on (modified_sequence, is_decoy) - both target/decoy-specific, so a target never inherits its paired decoy's good run. An empty modified_sequence (a stub loaded without the column) has no peptide identity and is skipped. The clamp runs at the end of run_percolator_fdr for both the direct and streaming paths (covering first-pass and the reconciliation-aware second pass), and again unconditionally on the final post-reconciliation pool right before the blib output gate, since Stage-6 reconciliation resets the run q-value of moved / gap-fill peaks after the in-pass clamp. The two HPC --join-at-pass=2 recompute paths are left unclamped for now, each marked with a PARITY NOTE for a follow-up. This mirrors PercolatorEngine.ClampExperimentQToBestRun and its two call sites in the C# implementation, for cross-impl parity. * Percolator: always stream, remove the direct/small-experiment path [WIP] Unify on the streaming path regardless of experiment size: drop the use_streaming dispatch and the run_percolator_fdr_direct function. The direct path trained the SVM on ALL entries with a different standardizer-fit population; streaming subsamples best-per-precursor. There is no reason to hold every file's features resident, so removing the direct path lowers the memory ceiling and leaves a single code path. Mirrors the C# streaming-only change (ProteoWizard/pwiz branch Skyline/work/20260708_osprey_percolator_streaming_only) to keep the two implementations at cross-impl parity. Verified: end-to-end bit-parity with C# at 1e-9 on Stellar (57112 precursors, Stage-7 protein FDR + blib content match). cargo fmt / clippy -D warnings / cargo test all pass. * Condition intensity features with log10 to prevent Percolator hijack (#53) * Condition intensity features with log10 to prevent Percolator hijack peak_apex, peak_area, and peak_sharpness were emitted as raw linear intensities. The single experiment-wide Percolator standardizer maps a lone high-intensity DIA interference to a z-score of 100-300 that dominates the linear SVM discriminant, letting intensity outliers (a random mix of target / decoy / entrapment) hijack the top of the score ranking (floored q-values, a top-of-ranking FDP spike, and a full run collapsing to zero IDs at 1:1 entrapment). Apply log10(x + 1) to all three in their feature computations, matching Skyline mProphet's MQuestIntensityCalc. A linear SVM cannot learn a saturating transform on its own, so it must be applied in the feature. Contained to these PIN features; peak.area stays raw for quantification. Mirrors the paired ProteoWizard/pwiz change to keep C#/Rust cross-impl parity; verified with Compare-EndToEnd-Crossimpl at 1e-9 on Stellar and Astral. cargo fmt / clippy -D warnings / cargo test all pass. Co-Authored-By: Claude <noreply@anthropic.com> * Floor peak_sharpness at 0 before log10 to avoid a non-finite feature The apex is an override/CWT lookup, not the recomputed reference-XIC max, so a supplied apex below an edge yields a negative mean slope. (sharpness + 1.0).log10() is then non-finite for sharpness <= -1; flooring the input with sharpness.max(0.0) before the log keeps the feature well-defined. peak_apex and peak_area are >= 0 by construction and unchanged. Mirrors the paired ProteoWizard/pwiz change to keep C#/Rust cross-impl parity; re-verified with Compare-EndToEnd-Crossimpl at 1e-9 on Stellar and Astral. cargo fmt / clippy -D warnings / cargo test all pass. Co-Authored-By: Claude <noreply@anthropic.com> * Correct the join-at-pass=2 clamp comments The two PARITY NOTE comments at the --join-at-pass=2 recompute sites claimed the HPC path does not apply clamp_experiment_q_to_best_run and that a follow-up should add one. That overstates the gap. There is no early return between those recompute sites and the authoritative clamp on the final post-Stage-6 pool, and the only consumer of experiment q -- the blib output gate -- runs after it. (The protein report is written just before the clamp but takes parsimony and protein_fdr_result, not the entries' experiment q.) So the HPC path is clamped; the notes predate the authoritative clamp added later on this branch. Comment-only change; no behavior difference. Co-Authored-By: Claude <noreply@anthropic.com> * Address Copilot review: fix the v3 envelope rejection in the rescore worker hydrate_for_rescore hard-coded `format_version != 2` as a redundant second check. read_reconciliation_file already validates format_version against RECONCILIATION_FORMAT_VERSION, which this branch bumps to 3, so the literal check rejected every envelope the pipeline can now write -- the per-file rescore worker was dead on this branch and no test covers it, which is why CI stayed green. Removed the check rather than re-pinning it to 3: a duplicate version gate that must be kept in sync is exactly what drifted. Also: * Corrected the v2 changelog entry, which described `file_stems` as optional via serde(default) when it is required and v1 is rejected at deserialization. * Extracted condition_intensity_feature (log10(max(x, 0) + 1)) and unit-tested the contract: 0 -> 0, decade scaling, monotonicity, and negative inputs flooring to a finite 0. Flooring apex/area is a no-op (both are >= 0 by construction), so PIN values stay bit-identical to the C# side. Co-Authored-By: Claude <noreply@anthropic.com> * Fix clippy doc_lazy_continuation in condition_intensity_feature docs A doc line began with ">= 0", which markdown parses as a block-quote marker, so the following lines became lazy continuations and clippy::doc_lazy_continuation fired under -D warnings. Reworded to spell out "non-negative" instead of opening a line with the >= operator. Comment-only; no behavior change. --------- Co-authored-by: brendanx67 <brendanx@proteinms.net> Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Co-authored-by: Brendan MacLean <brendanx67@users.noreply.github.com>
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.
Mirrors the C# Osprey experiment-q "best-of-runs" clamp (ProteoWizard/pwiz#4390) so the two implementations stay in cross-impl parity.
Why
Experiment-level FDR competes each precursor's single best observation against a de-duplicated (thinner) decoy null, so the raw experiment q-value can fall below every per-run q-value. That lets a precursor pass experiment-level FDR with no single run passing run-level FDR — producing reported peptides with no run-level ID (a blib ID-line artifact) and an anti-conservative experiment-wide FDP.
What
Adds
clamp_experiment_q_to_best_run(inpipeline.rs), which floors each entry'sexperiment_precursor_qvalue/experiment_peptide_qvalueup to that entry's best (min-over-runs) combined run q-value (FdrLevel::Both = max(run_precursor, run_peptide)), i.e. a best-of-runs aggregate can never be more confident than its best single run. Keying:entry_id(decoy high bit included),(modified_sequence, is_decoy)— both target/decoy-specific, so a target never inherits its paired decoy's good run; an emptymodified_sequence(a stub loaded without the column) is skipped.Two call sites, mirroring the C#:
run_percolator_fdr(both the direct and streaming paths, covering first-pass and the reconciliation-aware second pass);The two HPC
--join-at-pass=2recompute paths are left unclamped for now, each marked with a// PARITY NOTEfor a follow-up.Validation
Compare-EndToEnd-Crossimplshows the C# (pwiz#4390) and this branch remain at end-to-end bit-parity at 1e-9 on Stellar (56,534 precursors) and Astral (160,358) — Stage-7 protein FDR and full blib content match.cargo fmt/clippy -D warnings/cargo testall pass.Companion to the C# PR; opened against
reconciliation-v3-first-pass-base-ids(the current parity base) for a clean single-commit diff.