Skip to content

Use Rust FDR kernels in perform_fdr with exact tie-break handling - #841

Draft
GeorgWa wants to merge 3 commits into
mainfrom
feature/rust-qval-calc
Draft

Use Rust FDR kernels in perform_fdr with exact tie-break handling#841
GeorgWa wants to merge 3 commits into
mainfrom
feature/rust-qval-calc

Conversation

@GeorgWa

@GeorgWa GeorgWa commented Aug 24, 2026

Copy link
Copy Markdown
Collaborator

Uses the Rust FDR kernels for the hot spots in perform_fdr, keeping the pandas implementation as the reference path:

  • fdr_q_values / fdr_keep_best replace the pandas multi-key sort + cumsum + groupby in get_q_values and keep_best.
  • fdr_finalize replaces the sort -> get_q_values -> keep_best -> get_q_values chain with a single counting pass, used when fragment competition is not required.
  • ALPHADIA_RUST_FDR=0 forces the reference path for A/B benchmarking, and it is also used automatically when the kernels are unavailable.

get_q_values needs an integer tie-break key for the kernel. Precursor FDR breaks ties on precursor_idx, but protein FDR breaks ties on pg, a string accession. Rather than branching on the column's dtype, _integer_tiebreak factorizes non-integer columns with sort=True, which numbers the unique values in sorted order, so ordering by the codes reproduces ordering by the original values and the kernel stays exact for any sortable dtype. Missing values map to the largest key so they sort last, as pandas sorts them.

Requires MannLabs/alphadia-search-rs#130. Until those kernels ship in a released alphadia-search-rs, _RUST_FDR_AVAILABLE is False, the reference path runs, and test_fdr_rust_parity.py skips.

Replaces #820, which was stacked on an unreviewed branch; this branch is cut from a clean main.

🤖 Generated with Claude Code

GeorgWa and others added 3 commits August 24, 2026 18:10
Wires the alphadia_search_rs FDR kernels into alphadia/fdr/fdr.py:

- get_q_values and keep_best delegate to the Rust kernels when available.
- perform_fdr routes through the fused sort-free fdr_finalize whenever
  no fragment competition is needed (the common path), computing
  keep-best and q-values in a single counting pass. The fragment
  competition path retains the exact (now parallel-sorted) chain.

Guarded by availability and ALPHADIA_RUST_FDR (set to 0 to force the
reference pandas implementation for A/B benchmarking). On a 13M-row
batch this cuts FDR finalization from ~39s to ~1.2s (~32x); survivors
match exactly and q-values agree with the pandas reference to ~3e-5.

Adds parity tests comparing the Rust path against the pandas reference.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
perform_protein_fdr calls get_q_values with extra_sort_columns=["pg"],
the protein-group accession (a string). The Rust q-value path coerced
that column to int64, raising ValueError during protein FDR. Take the
Rust path only when the tie-break column is integer-typed; otherwise
fall back to the reference pandas sort (string-safe).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The Rust q-value kernel takes an integer tie-break key, so the previous guard
sniffed the column's dtype and fell back to pandas for protein FDR, which breaks
ties on the protein group accession. Inferring the code path from a dtype means a
future integer accession id would silently switch numerics.

Factorizing with sort=True numbers the unique values in sorted order, so ordering
by the codes reproduces ordering by the original values and the kernel stays exact
for any sortable dtype. Missing values are mapped to the largest key so they sort
last, as pandas sorts them. Only the kernel's single-key limit still routes
multi-column tie-breaks to the reference path.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@GeorgWa

GeorgWa commented Aug 24, 2026

Copy link
Copy Markdown
Collaborator Author

Context: why there is a tie-break at all, and an open question about removing it

Draft for now — see the status note at the bottom.

The tie-break

get_q_values is order-dependent: it sorts by score, accumulates target/decoy counts, computes cumsum(decoy) / cumsum(target), and converts to q-values with a reverse running minimum. Every step after the sort depends on the sequence of rows, so two rows with an equal score have no defined position relative to each other — and the numbers move depending on which one is picked. Five PSMs with identical scores, differing only in input order:

targets first  decoy=[0 0 0 1 1]  qval=[0.    0.    0.    0.333 0.667]
interleaved    decoy=[0 1 0 1 0]  qval=[0.    0.5   0.5   0.667 0.667]
decoys first   decoy=[1 1 0 0 0]  qval=[0.667 0.667 0.667 0.667 0.667]

Hence the three sort keys: score for the ranking, decoy ascending so targets precede decoys within a tie, and extra_sort_columns to make the order total so it is a property of the data rather than of how the frame was assembled upstream.

Ties are not hypothetical. Classifier probabilities saturate to exactly 0.0 / 1.0 for confident predictions, and in protein FDR the features (count, n_peptides, n_precursor, n_runs, best/mean/worst score) are identical across single-peptide proteins, so identical inputs give bit-identical probabilities in the crowded region where the threshold lives.

What this PR does

The Rust kernel takes the tie-break as &[i64], so precursor_idx passes through but pg (a string accession) raised ValueError. Rather than branching on dtype — which would silently reroute protein FDR, and silently reroute it back if pg ever became an integer id — _integer_tiebreak factorizes non-integer columns with sort=True. That numbers the unique values in sorted order, so ordering by the codes is equivalent to ordering by the originals and the kernel stays exact for any sortable dtype.

The requirement here is stronger than determinism: the pandas implementation remains a live path (always for fragment competition, and everywhere when the kernels are unavailable), so the two must agree numerically or the same input yields different q-values depending on whether the extension is installed.

Open question: one q-value per equal-score block

Assigning every row in an equal-score block the FDR evaluated at the block's end removes the tie-break entirely — no tie-break column, no factorize, no dtype question, and fdr_q_values loses its third argument. It also lets the kernel use an unstable single-key parallel sort, since intra-block order stops being load-bearing.

It is also more defensible. Within a tie block the current convention places every target ahead of every decoy, so the leading targets see cumsum(decoy) == 0 and get q = 0. Measured on 200k synthetic rows with a real target/decoy separation:

ties unique scores q<0.001 targets-first blocks delta
none (float64) 200,000 72,274 72,274 0
moderate (block ~21) 9,375 72,301 72,241 -60 (-0.08%)
dense (block ~2000) 100 78,025 72,164 -5,861 (-7.5%)

Bit-exact where there are no ties. On data with no signal at all and dense ties, targets-first reports 3,906 IDs at 0.1% FDR where the truth is 0 — those are manufactured by the sort convention.

Blocks would be grouped by exact float equality, deliberately: the ties we care about come from bit-identical classifier outputs, not from near-equality. Two scores differing by one ULP are genuinely different scores and belong in different blocks, so == on f64 is the correct predicate and a tolerance-based grouping would be a regression, not an improvement. Same reasoning applies to fdr_finalize's fixed-bin histogram, which is a different mechanism: it groups by value ranges, so it lumps genuinely distinct scores together and quantizes q-values to bin width. That approximation is acceptable at 10M+ candidates but is not what is being proposed here.

Not in this PR because it is a behaviour change, not a refactor: reported IDs will drop wherever ties are dense, the numbers above are synthetic distributions rather than alphaDIA data, and both implementations would have to change in the same commit. Wants an e2e comparison first.

Status

Draft until MannLabs/alphadia-search-rs#130 ships the kernels in a released alphadia-search-rs. Until then _RUST_FDR_AVAILABLE is False, every _rs_* branch is dead, the pandas reference runs, and test_fdr_rust_parity.py skips at module level — so this cannot be benchmarked or reviewed for parity yet.

Replaces #820, which was stacked on the unreviewed feature/rust-fdr; that branch and fix/rust-fdr-protein-qval-int64 have been deleted.

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.

1 participant