Percolator streaming-only, experiment q-value clamp, and log-conditioned intensity features by maccoss · Pull Request #55 · maccoss/osprey

maccoss · 2026-07-13T06:45:01Z

Summary

Flattens the percolator-streaming-only stack into main. The stack was built
under reconciliation-v3-first-pass-base-ids and never landed directly, so three
changes are still absent from main even though parts of the stack reached it by
other routes (#48, #50, #52). The net delta is confined to two files:
pipeline.rs (+234/-124) and reconciliation_io.rs (+73/-6).

Contents:

Percolator: always stream, remove the direct/small-experiment path (Percolator: always stream, remove the direct/small-experiment path #51).
Clamp the experiment q-value to each entry's best run q-value (Clamp experiment q-value to each entry's best run q-value (cross-impl parity with pwiz#4390) #49) --
cross-impl parity with pwiz#4390.
Condition the three intensity PIN features (peak_apex, peak_area,
peak_sharpness) with log10(x + 1), including the max(0.0) floor on sharpness
(Condition intensity features with log10 to prevent Percolator hijack #53) -- cross-impl parity with pwiz#4412, which is now merged to pwiz master.

Parity note: pwiz master already carries the C# side of both #49 and #53, so
main is currently behind the C# implementation on the PIN feature vector. A
cross-impl comparison run against main today would diverge on peak_apex,
peak_area, and peak_sharpness.

Relationship to #54: that PR carries the same log-conditioning change to main
plus a v26.7.0 release cut. If this lands first, #54 should be reduced to the
version bump and release notes, or closed as superseded.

Test plan

CI (fmt / clippy -D warnings / cargo test) green on this PR -- the branch
head has never been built by CI
Cross-impl gate (Compare-EndToEnd-Crossimpl, Stellar + Astral) against
pwiz master

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>

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.

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.

…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>

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>

Copilot

Pull request overview

This PR folds in the outstanding “percolator-streaming-only” stack changes onto main, bringing the Rust pipeline back into cross-implementation parity with the C# (pwiz) implementation for Percolator execution mode, experiment-level q-value clamping, and intensity feature conditioning.

Changes:

Remove the direct (non-streaming) Percolator path and always use the streaming workflow.
Clamp experiment-level q-values to each entry’s best run-level combined q-value, including an authoritative post-reconciliation re-clamp.
Add first_pass_base_ids to reconciliation.json (format v3) and apply log10(x+1) conditioning to intensity PIN features (with sharpness flooring).

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

File	Description
crates/osprey/src/pipeline.rs	Switches Percolator to streaming-only, adds experiment-q clamping (including final-stage re-clamp), writes v3 reconciliation envelopes with join-wide base IDs, and log-conditions intensity PIN features.
crates/osprey/src/reconciliation_io.rs	Bumps reconciliation format to v3 and adds required `first_pass_base_ids` field with deterministic serialization + updated tests.

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

maccoss · 2026-07-13T16:45:37Z

+///     pipeline. Required in v3 (no `#[serde(default)]`): v2 envelopes are
+///     rejected at parse time.
+pub const RECONCILIATION_FORMAT_VERSION: u32 = 3;


Confirmed, and this was a real break, not a style nit. read_reconciliation_file already validates format_version against RECONCILIATION_FORMAT_VERSION, so once that constant went to 3 the literal != 2 in hydrate_for_rescore rejected every envelope that can still parse — the per-file rescore worker was dead on this branch. No test covers hydrate_for_rescore with a real envelope, which is why CI stayed green.

Fixed in 82cb59d by removing the check outright rather than re-pinning it to 3: a duplicate version gate that has to be kept in sync with the constant is exactly the thing that drifted here. read_reconciliation_file is the single gate, and serde rejects legacy envelopes on the missing required field (v1: no file_stems; v2: no first_pass_base_ids).

The test gap is real and is not closed by this commit — hydrate_for_rescore needs an end-to-end envelope test. Tracking separately rather than expanding this PR.

maccoss · 2026-07-13T16:45:38Z

@@ -228,7 +245,13 @@
 ///     parquet). Old v1 files deserialize with empty `file_stems` via
 ///     `#[serde(default)]`; the worker falls back to its
 ///     `config.input_files` stems for those (preserving v1 behavior).


Correct — fixed in 82cb59d. file_stems is required (no #[serde(default)]) and v1 envelopes are rejected at deserialization, so the v2 changelog entry describing a fallback to config.input_files stems was describing behavior that no longer exists. Reworded to match.

maccoss · 2026-07-13T16:45:39Z

+        peak_apex: (peak.apex_intensity + 1.0).log10(),
+        peak_area: (peak.area + 1.0).log10(),
        peak_width,
        peak_symmetry,
        signal_to_noise: peak.signal_to_noise,
        n_scans: peak_len as u16,
-        peak_sharpness,
+        // Floor sharpness at 0 BEFORE the log: the apex is an override/CWT lookup,
+        // not the recomputed reference-XIC max, so an apex below an edge yields a
+        // negative mean slope; max(0.0) keeps the argument >= 1 (result-flooring
+        // log10 of a negative is NaN). apex/area are >= 0 by construction.
+        peak_sharpness: (peak_sharpness.max(0.0) + 1.0).log10(),


Fair, and taken. Extracted condition_intensity_feature(x) = log10(max(x, 0) + 1) and added condition_intensity_feature_floors_logs_and_stays_finite in 82cb59d, covering exactly the contract you list: 0 -> 0 (the invalid-peak sentinel survives), decade scaling, strict monotonicity across a four-orders-of-magnitude spread (compressing 1e7 to < 8, which is the point — the outlier no longer standardizes to z = 100-300), and negative inputs flooring to a finite 0.

The negative case is the one that matters: peak_sharpness can genuinely go negative because its apex is the override/CWT lookup rather than the recomputed reference-XIC max, and the naive (x + 1).log10() is NaN below -1. The floor has to come first — max(0.0, NaN) is still NaN — and the test pins that ordering.

One deliberate change beyond the extraction: the helper floors peak_apex and peak_area too. Both are >= 0 by construction (XIC intensities are raw centroids, never smoothed or background-subtracted — the background subtraction in compute_snr operates on a scalar and never rewrites the array), so the floor is a no-op and PIN values stay bit-identical to the C# side. It just enforces an invariant that was previously only assumed.

…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>

main had advanced with the calibration pass-2 refit fix (#52) and the gap-fill isolation m/z filter (#50). One conflict, in the reconciliation_io.rs v2 changelog comment: main still carries the stale text describing file_stems as optional via serde(default), which this branch corrected per the Copilot review. Kept the corrected text -- file_stems is required and v1 envelopes are rejected at deserialization on both sides.

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.

…otes #55 landed the streaming-only Percolator, the best-of-runs experiment q-value clamp, the log conditioning, and the hydrate_for_rescore fix, so this branch's own copy of condition_intensity_feature was a duplicate. Resolved the pipeline.rs conflict onto main's merged helper, kept this branch's granular unit tests, and dropped the redundant consolidated one. Release notes were written before #55 and described only the log conditioning. Added the two missing Scoring/FDR entries -- the experiment q-value clamp (which CHANGES REPORTED Q-VALUES: expect slightly higher experiment q and fewer IDs at a fixed threshold than 26.6.x, the old numbers being optimistic) and the streaming-only Percolator -- plus the hydrate_for_rescore fix under Bug Fixes. Documented the clamp in docs/07-fdr-control.md, and corrected that doc's subsampling section, which still described streaming as a size-conditional path. Also corrected the helper doc: it claimed max(0, NaN) is NaN, which is true in C# but NOT in Rust (f64::max ignores NaN). The input-flooring order is load-bearing for C# parity, not for Rust's own safety; the NaN test now says so, and flags that the two implementations would silently disagree on a NaN input.

Cuts v26.7.0: bumps the version, finalizes the release notes, and updates the docs. The log-conditioning code itself landed in #55 (along with the streaming-only Percolator, the best-of-runs experiment q-value clamp, and the hydrate_for_rescore fix), so this PR's duplicate copy of condition_intensity_feature was resolved onto main's merged helper. Its granular unit tests for the helper were kept. Release notes now cover the full release, including the two entries written after the original cut: the experiment q-value clamp (which changes reported q-values -- expect slightly higher experiment q and fewer IDs at a fixed threshold than 26.6.x) and the streaming-only Percolator. docs/07-fdr-control.md documents the clamp and no longer describes streaming as a size-conditional path. Co-Authored-By: Claude <noreply@anthropic.com>

brendanx67 and others added 4 commits July 2, 2026 18:52

Copilot AI review requested due to automatic review settings July 13, 2026 06:45

Copilot started reviewing on behalf of maccoss July 13, 2026 06:45 View session

Copilot AI reviewed Jul 13, 2026

View reviewed changes

maccoss and others added 3 commits July 13, 2026 09:45

maccoss merged commit 321449b into main Jul 13, 2026
3 checks passed

maccoss mentioned this pull request Jul 13, 2026

Cover hydrate_for_rescore with a real test, replacing one that could not fail #56

Merged

4 tasks

Navigation Menu

Percolator streaming-only, experiment q-value clamp, and log-conditioned intensity features - #55

Percolator streaming-only, experiment q-value clamp, and log-conditioned intensity features#55
maccoss merged 8 commits into
mainfrom
percolator-streaming-only

maccoss commented Jul 13, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

maccoss Jul 13, 2026

Uh oh!

maccoss Jul 13, 2026

Uh oh!

maccoss Jul 13, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

maccoss commented Jul 13, 2026

Summary

Test plan

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

maccoss Jul 13, 2026

Choose a reason for hiding this comment

Uh oh!

maccoss Jul 13, 2026

Choose a reason for hiding this comment

Uh oh!

maccoss Jul 13, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants