Repository navigation
fix(imspy-simulation): never write a non-finite precursor intensity (#488) - #489
Merged
Merged
Conversation
…488) A peptide with many basic sites puts most of its binomial charge mass above max_charge, which is truncated away, so every remaining state can fall below min_charge_contrib. The charge-state normalisation then divided by a zero kept mass: every state became inf, inf passed the threshold, and the DDA precursor intensity (relative_abundance * events * frame_abundance) reached Precursors.Intensity as inf. timsrust >= 0.6 reads TDF rows through JSON, which has no infinity, and rejects the whole table. - predictors: factor the selection into kept_charge_states(). When no state passes the threshold, keep the most probable charge at abundance 1.0 instead of dividing by zero (old) or silently dropping the peptide (current guard). - tdf: write_precursor_table refuses inf/NaN in m/z, scan and intensity columns (MonoisotopicMz may be NULL, never inf), naming the offending Ids, so any future upstream source fails the run instead of emitting an unreadable .d. Tests cover the zero-mass fallback, the binomial model on the peptide from the affected HLA fixture, and the writer guard.
…-degenerate path Review follow-up. kept_charge_states() applied the threshold before rescaling, while the original loop divides all charged states by the kept mass and then thresholds the rescaled values, so peptides whose low-probability states cross the threshold after rescaling lost ions. Restore the original arithmetic verbatim (np.sum, rescale-then-threshold) and confine the change to the zero-kept-mass case under normalisation: - the fallback no longer fires with normalize=False, where nothing is divided - an all-zero charged vector behaves as before (finite zeros at threshold 0) - the fallback picks the largest finite state, never a NaN position - validate_precursor_values accepts pandas nullable Float64 with missing m/z Tests now compare the helper against a verbatim copy of the old loop on 5000 random distributions and on binomial outputs.
… the old loop did
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
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.
Fixes the simulator side of #488: some simulated DDA
.dfiles carriedPrecursors.Intensity = inf, which timsrust >= 0.6 rejects (it reads TDF rows through JSON, which has no infinity).Root cause
BinomialChargeStateDistributionModelrenormalises the charge states that passmin_charge_contrib. A peptide with many basic sites (the affected HLA peptideAVGKNKRLTKGGKKGAKKKVVDhas nine) puts most of its binomial mass abovemax_charge, which is truncated away, so every remaining state falls below the threshold and the kept mass is zero. Before January 2026 the loop divided by it: every state becameinf,infpassed the threshold, andrelative_abundance * events * frame_abundancereached the precursor table asinf. The current guard avoids the division, but the peptide is then silently dropped.Changes
imspy_predictors.ionization.predictors: the selection moves intokept_charge_states(). When the kept mass is zero under normalisation, the peptide is kept at its most probable finite charge with abundance 1.0. Every other path is the original arithmetic unchanged (rescale all states, then threshold; samenp.sum, same dtype), checked against a verbatim copy of the old loop on 5000 random distributions and on binomial outputs.imspy_simulation.tdf:write_precursor_tablecalls a newvalidate_precursor_values(), which refusesinf/NaNin the m/z, scan and intensity columns (a missingMonoisotopicMzis still allowed) and names the offending Ids. Any future upstream source of a non-finite value then fails the run instead of producing a.dthat some readers reject.Verification
Float64.imspy-predictors/tests/test_predictors.py+ new tests + all ofimspy-simulation/tests: 249 passed, 15 skipped (hardware/SDK-gated), 0 failed. Same failing set asmainin a reduced environment, i.e. no regressions.Code review
Two Codex review rounds. Round 1 caught a real regression in the first version of the helper (it thresholded before rescaling, which changed the ions of ordinary peptides) plus three edge cases (zero threshold,
normalize=False, nullable dtype); all are fixed in a153ae7. Round 2 confirmed those fixes and flagged a dtype-parity nit, fixed in 183f2d3.Out of scope, noted by the review: other TDF metadata writers (
PasefFrameMsMsInfo, DIA windows) have no finite-value guard yet; no current path is known to feed them non-finite values.The affected published fixtures (SAGEBench HLA bundles on Zenodo) are corrected separately as a new record version; no re-simulation is needed.