Skip to content

Select proteoform candidates by the most abundant isotopic peak#2671

Open
trishorts wants to merge 28 commits into
smith-chem-wisc:masterfrom
trishorts:mostAbundantMass
Open

Select proteoform candidates by the most abundant isotopic peak#2671
trishorts wants to merge 28 commits into
smith-chem-wisc:masterfrom
trishorts:mostAbundantMass

Conversation

@trishorts

Copy link
Copy Markdown
Contributor

Summary

Adds an opt-in PrecursorMassMatchMode (default Monoisotopic) that, when set to MostAbundant, selects top-down candidates by matching the observed most abundant isotopic peak against each candidate's theoretical apex (exact monoisotopic + averagine offset), instead of the inferred monoisotopic mass. This directly removes the monoisotopic off-by-N errors that cap high-mass proteoform identification.

Why a notch set

A single tight-ppm point at the averagine-predicted apex under-identifies, because the predicted apex misses the experimental apex by ±1–2 neutrons (~67 ppm/neutron at 15 kDa). MostAbundantMassDiffAcceptor therefore emits a small notch set at apex + k·(13C−12C), k ∈ {−2..+2}, each at tight ppm, keeping FDR controlled per notch.

Changes

  • PrecursorMassMatchMode enum; CommonParameters.PrecursorMassMatchMode (+ toml/clone/file-specific plumbing).
  • Ms2ScanWithSpecificMass.PrecursorMassToMatch (defaults to PrecursorMass, which is left untouched for fragment math/error reporting).
  • _GetMs2Scans selects the match mass from the envelope (most-abundant, or average if unresolved).
  • New MostAbundantMassDiffAcceptor; SearchTask.GetMassDiffAcceptor returns it when enabled. Classic/Modern search route candidate selection through PrecursorMassToMatch.

Validation (yeast top-down, 1% FDR, pooled across all 24 fractions)

MostAbundant 326 proteoforms / 10,850 PSMs vs monoisotopic ±3-notch baseline 281 / 9,314+16.0% / +16.5%, using fewer notches (5 vs 7). Consistent at 8-fraction (+15.1%) and single-fraction scale.

Notes

  • Depends on Expose the most-abundant observed mass on IsotopicEnvelope (resolved) mzLib#1078 (most-abundant/average envelope masses + averagine offsets). Draft until that merges and a mzLib release is available; the mzLib PackageReference version here is a local build and must be bumped to the released version before merge.
  • Centroid-mass path for isotopically unresolved (>35 kDa) species is plumbed but dormant pending an unresolved charge-determination algorithm (future work).

🤖 Generated with Claude Code

trishorts and others added 11 commits June 16, 2026 13:58
…peak

Add an opt-in PrecursorMassMatchMode (default Monoisotopic) that, when set to
MostAbundant, selects theoretical candidates by matching the observed most
abundant isotopic peak against each candidate's theoretical most-abundant mass
(exact monoisotopic + averagine offset) via a new MostAbundantMassDiffAcceptor.
This directly avoids the off-by-N precursor errors that arise for high-mass
proteoforms whose monoisotopic peak is undetectable, and replaces the
missed-monoisotopic notches when enabled.

Scans carry a separate PrecursorMassToMatch (the most-abundant or, for
unresolved species, average observed mass); PrecursorMass is unchanged so
fragment-bin math and mass-error reporting are unaffected. Classic and modern
search route candidate selection through PrecursorMassToMatch. Centroid/average
handling for isotopically unresolved species is plumbed but dormant pending the
v2 unresolved charge-determination algorithm.

Requires mzLib most-abundant/average envelope masses and averagine offsets
(local mzLib 9.9.25).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The averagine-predicted most-abundant isotopologue can differ from the
experimental apex by 1-2 neutrons (~67 ppm/neutron at 15 kDa), so a single
tight-ppm point at the predicted apex misses many correct candidates. Emit a
discrete notch set at apex + k*C13 for k in [-2..+2], each at tight ppm, so
apex misprediction is tolerated while FDR stays controlled per notch. Notch
integers use the existing round(shiftDa*NotchScalar) convention so they never
collide with the -1 "not accepted" sentinel.

Validated on yeast top-down (fract8): 114 proteoforms / 765 PSMs at 1% FDR,
vs 99/713 for the production ThreeMM monoisotopic baseline (with fewer notches)
and 93 for the single-notch monoisotopic control.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Calibration now uses MostAbundantMassDiffAcceptor when PrecursorMassMatchMode is
MostAbundant; its mass-error data points are recomputed per-isotope from the
identified peptide's theoretical distribution, so the calibration function is
unaffected by the match convention. GPTMD gets a new MostAbundantDotMassDiffAcceptor
that composes its modification-mass shift set with the apex offset and the +/-2
neutron apex notch set (the shift's notch is shared across apex sub-notches).
Both tasks route candidate selection through PrecursorMassToMatch.

Verified end-to-end on a yeast fraction: Cal->GPTMD->Search runs to completion in
most-abundant mode (calibration found 765 PSMs / 64k datapoints).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
GptmdParameters.GptmdFilters is a List<IGptmdFilter> that cannot round-trip
through the Nett toml, so filters like ImprovedScoreFilter (add a modification
only when it improves the match score) were settable only in the GUI. Add a
toml-serializable GptmdFilterTypes (list of filter type names) resolved to
filter instances via GetActiveFilters(); mark GptmdFilters [TomlIgnore]. The
GUI now persists its selection into GptmdFilterTypes so it survives save/load
and CMD runs. Headless top-down workflows can now set
GptmdFilterTypes = ["ImprovedScoreFilter"] to greatly limit modifications added.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… add

The prior commit referenced GptmdParameters.GetActiveFilters()/GptmdFilterTypes
but the file was not staged (filename case mismatch on git add), leaving the
branch tip non-compiling. Add the GptmdParameters changes.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…e doesn't write runaway tolerances

Calibration set its output precursor tolerance from 3*IQR + |median| of the PSM
precursor errors, computed as monoisotopic-observed minus peptide-monoisotopic.
Under most-abundant precursor matching, candidates are matched on the apex, so the
reported monoisotopic is intentionally +/-1-2 neutrons off; those integer offsets
inflated the IQR and made calibration write enormous tolerances (observed up to
1940 ppm on a yeast fraction), which then exploded the candidate space and memory
in downstream GPTMD/Search.

Strip the nearest-neutron component from each PSM precursor error before taking the
IQR/median, leaving the true sub-ppm drift. This is a no-op for monoisotopic-matched
PSMs (verified: monoisotopic tolerances unchanged; most-abundant fract9/fract11 went
from 1940/141 ppm to 2.9/2.2 ppm). Also clamp the written tolerance to never exceed
the searched tolerance as a guardrail.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… clamp

Two unit tests for the most-abundant calibration fix: (1) PSMs with ±1–2 neutron
offsets between the deconvoluted monoisotopic and the peptide monoisotopic yield a
few-ppm precursor IQR (offsets stripped), and (2) calibration never writes a
precursor tolerance wider than the one it searched with.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…rops GetMostAbundantOffset

mzLib's GetMostAbundantOffset was removed in PR smith-chem-wisc#1078 review; the most-abundant
acceptors now compute the apex offset via a private ApexOffset helper that composes
the existing AverageResidue API (GetDiffToMonoisotopic(GetMostIntenseMassIndex)).
Bumps local mzLib reference to 9.9.26. Tests green.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Address findings from the PR smith-chem-wisc#2671 review.

Search / acceptor correctness:
- ModernSearch FineScorePeptide assigned the authoritative notch with
  scan.PrecursorMass while coarse selection used PrecursorMassToMatch; in
  most-abundant mode these differ by the averagine apex offset, so survivors
  were re-scored against the wrong mass and got notch -1/wrong, defeating
  per-notch FDR. Use PrecursorMassToMatch for the notch.
- MostAbundantMassDiffAcceptor encoded notches as round(k*C13*NotchScalar),
  yielding negative ids for k<0 that ModernSearch's Accepts(...)>=0 guard
  drops. Encode each apex offset as its contiguous 0-based index ({0..4});
  FDR groups by notch value, so validated ClassicSearch results are unchanged
  while the low-side apex matches survive.
- Guard ApexOffset against non-positive shifted mass in both most-abundant
  acceptors (large negative GPTMD shift on a small candidate).
- Rename the FromObservedMass parameter to observedMostAbundantMass.

Calibration (gate guardrails on most-abundant; preserve default baseline):
- Gate the neutron-modulo precursor-error strip on most-abundant mode
  (threaded through DataPointAquisitionResults); default Monoisotopic
  calibration now uses the raw difference exactly as before.
- Gate the tolerance clamp in UpdateCombinedParameters on most-abundant mode.
- Use the apex acceptor only when deconvolution is on; calibration runs
  deconvolution off, so fall back to the zero-centered acceptor where
  PrecursorMassToMatch is the monoisotopic mass.
- Round the neutron count AwayFromZero (parity-independent half-neutron).

Other:
- GPTMDTaskWindow: null-guard GptmdFilters.Any(...) like its sibling to avoid
  an NRE when the window opens against a filters-null toml.
- _GetMs2Scans: leave precursorMassToMatch null in monoisotopic mode so it
  defaults to PrecursorMass from one source of truth.
- Restore the UTF-8 BOM on six .csproj files (revert stylistic churn).
- Update CalibrationTests to exercise the gated guardrails in most-abundant
  mode.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ution

Add three unit tests for previously-uncovered most-abundant code paths:
- MostAbundantMassDiffAcceptor.GetAllowedPrecursorMassIntervalsFromObservedMass
  (the indexed/ModernSearch path) recovers the candidate mono window.
- MostAbundantDotMassDiffAcceptor theoretical and observed interval methods
  plus the far-candidate reject path.
- GptmdParameters.CreateFilter resolves every known filter name and returns
  null otherwise.

Lifts the two acceptors and GPTMDParameters above the 90% line.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Mirror the mzLib split: keep this PR on the resolved most-abundant path and
defer the isotopically-unresolved (average/centroid) path to future work.

- _GetMs2Scans now selects PrecursorMassToMatch from the most-abundant observed
  peak only; drop the EnvelopeResolution/AverageObservedMass branch. Guard on
  MostAbundantObservedMass > 0 so an envelope with no observed peak (the -1
  sentinel, e.g. a neutral mass read from a pre-deconvoluted file) falls back to
  the monoisotopic PrecursorMass.
- Update the MostAbundantMassDiffAcceptor remarks: the unresolved average-mass
  path is future work; no doc reference to the moved GetAverageOffset.

Build green; MostAbundantMassTest + CalibrationTests pass (40).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011QVxqemPqC9BjvYtVg6aKY
trishorts and others added 2 commits June 29, 2026 14:51
Update the consumer references in _GetMs2Scans to the renamed mzLib property
(MostAbundantObservedMass -> MostAbundantObservedNeutralMass). Mechanical rename;
no behavior change. Builds once the renamed mzLib package is referenced (the
mzLib PackageReference is the usual local placeholder bumped at merge).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011QVxqemPqC9BjvYtVg6aKY
# Conflicts:
#	MetaMorpheus/EngineLayer/CommonParameters.cs
#	MetaMorpheus/EngineLayer/Ms2ScanWithSpecificMass.cs
#	MetaMorpheus/TaskLayer/MetaMorpheusTask.cs
@codecov

codecov Bot commented Jun 30, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 90.74890% with 21 lines in your changes missing coverage. Please review.
✅ Project coverage is 93.30%. Comparing base (3c8e0b8) to head (92e13e0).

Files with missing lines Patch % Lines
...er/PrecursorSearchModes/PrecursorMassExtensions.cs 77.77% 4 Missing and 2 partials ⚠️
...pheus/TaskLayer/CalibrationTask/CalibrationTask.cs 81.25% 0 Missing and 3 partials ⚠️
...cificEnzymeSearch/NonSpecificEnzymeSearchEngine.cs 50.00% 0 Missing and 2 partials ⚠️
...rsorSearchModes/MostAbundantDotMassDiffAcceptor.cs 94.44% 0 Missing and 2 partials ⚠️
...ecursorSearchModes/MostAbundantMassDiffAcceptor.cs 93.93% 0 Missing and 2 partials ⚠️
MetaMorpheus/TaskLayer/GPTMDTask/GPTMDTask.cs 71.42% 0 Missing and 2 partials ⚠️
MetaMorpheus/TaskLayer/SearchTask/SearchTask.cs 71.42% 0 Missing and 2 partials ⚠️
MetaMorpheus/EngineLayer/Gptmd/GptmdEngine.cs 92.85% 0 Missing and 1 partial ⚠️
MetaMorpheus/EngineLayer/SpectralMatch.cs 95.45% 0 Missing and 1 partial ⚠️
Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff            @@
##           master    #2671    +/-   ##
========================================
  Coverage   93.29%   93.30%            
========================================
  Files         196      199     +3     
  Lines       21320    21495   +175     
  Branches     3994     4037    +43     
========================================
+ Hits        19891    20055   +164     
  Misses        893      893            
- Partials      536      547    +11     
Flag Coverage Δ
unittests 93.30% <90.74%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
...ineLayer/Calibration/DataPointAcquisitionEngine.cs 97.53% <100.00%> (+0.01%) ⬆️
...ineLayer/Calibration/DataPointAquisitionResults.cs 100.00% <100.00%> (ø)
...s/EngineLayer/ClassicSearch/ClassicSearchEngine.cs 99.39% <100.00%> (ø)
MetaMorpheus/EngineLayer/CommonParameters.cs 99.52% <100.00%> (+1.95%) ⬆️
.../EngineLayer/HistogramAnalysis/BinTreeStructure.cs 88.16% <100.00%> (+0.14%) ⬆️
...eus/EngineLayer/Localization/LocalizationEngine.cs 96.82% <100.00%> (+0.15%) ⬆️
...eus/EngineLayer/ModernSearch/ModernSearchEngine.cs 77.72% <100.00%> (ø)
...etaMorpheus/EngineLayer/Ms2ScanWithSpecificMass.cs 95.74% <100.00%> (+0.14%) ⬆️
...Morpheus/EngineLayer/SpectrumMatch/PsmTsvWriter.cs 97.58% <100.00%> (+0.02%) ⬆️
...ons/ViewModels/Deconvolution/DeconHostViewModel.cs 100.00% <100.00%> (ø)
... and 11 more

... and 1 file with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

trishorts and others added 3 commits June 30, 2026 11:47
…sk windows

Surface PrecursorMassMatchMode (already supported via CLI/TOML) in the GUI as a
'Use Most Abundant Mass' checkbox in the Calibration, GPTMD, and Search task
windows. Wiring is additive: DeconHostViewModel gains an optional
precursorMassMatchMode ctor arg (defaults Monoisotopic) plus a UseMostAbundantMass
boolean projection for the checkbox; each window reads the mode in
UpdateFieldsFromTask and writes it into CommonParameters on save. The shared
HostDeconParamControl and the other task windows are left untouched. Covered by a
DeconHostViewModel unit test (default, ctor passthrough, two-way projection).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Add changed-line tests for the most-abundant feature so codecov/patch clears:
- acceptor MaxApexOffsetNeutrons/ToString/ToProseString and the guarded <=0
  ApexOffset branch (MostAbundant + MostAbundantDot acceptors)
- GptmdParameters.GetActiveFilters with null collections (fallback branches)
- MetaMorpheusTask.GetMs2Scans in most-abundant mode resolves the deconvoluted
  envelope apex into PrecursorMassToMatch
- CalibrationTask end-to-end in most-abundant mode (apex acceptor selection)

Patch coverage 87.9% -> ~94% (Coverlet/Release proxy).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… margin

Add a GPTMD end-to-end test in most-abundant mode so the GPTMDTask apex notch/
prose acceptor branch is exercised, lifting PR patch coverage further above the
codecov/patch target.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Alexander-Sol
Alexander-Sol previously approved these changes Jul 6, 2026
Comment thread MetaMorpheus/EngineLayer/PrecursorSearchModes/MostAbundantMassDiffAcceptor.cs Outdated
Comment thread MetaMorpheus/TaskLayer/GPTMDTask/GPTMDParameters.cs
Comment thread MetaMorpheus/EngineLayer/Ms2ScanWithSpecificMass.cs Outdated
…rams

Addresses @nbollis round-1 review on PR smith-chem-wisc#2671.

- Replace the hardcoded Constants.C13MinusC12 in both most-abundant mass-diff
  acceptors with an ExpectedIsotopeSpacing arg, threaded from the existing
  DeconvolutionParameters.ExpectedIsotopeSpacing at every construction site
  (Calibration / GPTMD / Search). Removes the carbon-only spacing assumption
  the reviewer flagged for RNA/decoy runs; defaults to C13MinusC12 so no
  existing caller or test changes.
- Revert the unrelated GPTMD filter-serialization feature (GptmdFilterTypes /
  GetActiveFilters / CreateFilter / [TomlIgnore]) out of this PR to be
  resubmitted on its own branch. No property was renamed; the interface-typed
  GptmdFilters never round-tripped through Nett, so old tomls are unaffected.
- Expand the PrecursorMassToMatch doc to explain why it stays separate from the
  monoisotopic PrecursorMass (fragment-bin math and mass-error reporting remain
  monoisotopic-vs-monoisotopic, uncoupled from the per-notch apex offset).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Adds an optional "Most Abundant Mass Diff (ppm)" column, the most-abundant-mode
analogue of the monoisotopic "Mass Diff (ppm)". It reports the observed
most-abundant peak vs the candidate's theoretical averagine-apex mass, so a tight
value confirms the apex match and a non-tight value flags an apex misprediction
the notch tolerated — complementing the monoisotopic column, which instead
reflects deconvolution's monoisotopic-peak assignment.

- SpectralMatch.MostAbundantMassErrorPpm: null outside most-abundant mode.
- Extracted MostAbundantMassDiffAcceptor.AveragineApexOffset(...) as a shared
  static so selection and reporting use the identical apex model.
- Gated by the existing data-driven optional-column idiom, so normal
  monoisotopic output is unchanged.
- Write-only + MM-only: mzLib's psmtsv reader maps known headers by name and
  ignores unknown columns, so no mzLib change is needed and smith-chem-wisc#2671 keeps
  consuming released mzLib 1.0.581.

Addresses @nbollis review comment on precursor mass-error reporting (smith-chem-wisc#2671).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Comment thread MetaMorpheus/EngineLayer/SpectrumMatch/PsmTsvWriter.cs Outdated
…n header

Bump the mzLib PackageReference from 1.0.581 to the released 1.0.582, which
registers the "Most Abundant Mass Diff (ppm)" column in the canonical psmtsv
header set (SpectrumMatchFromTsvHeader.MostAbundantMassDiffPpm, mzLib smith-chem-wisc#1086).

- PsmTsvWriter now writes the value keyed by the mzLib-owned constant instead
  of a MetaMorpheus-local literal, and the MM-local MostAbundantMassDiffPpmHeader
  const is removed. psmtsv column names are owned by mzLib; MetaMorpheus only
  computes and writes the value. The column also now round-trips through mzLib's
  reader rather than being write-only.
- Remove a dead `using ThermoFisher.CommonCore.Data.Business;` from DIAEngine.cs.
  1.0.582 also carries the Thermo CommonCore RawFileReader 8.0.37 upgrade (smith-chem-wisc#1091),
  whose Business namespace now contains a Tolerance type; the unused using made
  MzLibUtil.Tolerance ambiguous. The DIA code only ever meant MzLibUtil.Tolerance.

Build 0 errors; MostAbundantMass 16/16, psmtsv suite 36/36.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Alexander-Sol
Alexander-Sol previously approved these changes Jul 10, 2026
RayMSMS
RayMSMS previously approved these changes Jul 10, 2026

@nbollis nbollis left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My beef with this implementation is that the Ms2ScanWithSpecificMass and SpectralMatch representations depend upon the state of the search engine when they should be stateless for the search type being performed. The addition of these new properties introduces many potential bugs and sources of error.

For example, the localization engine calculates the mass difference based upon psm.ScanPrecursorMass and peptide.MonoisotpicMass. Now the localization engine will be wrong if we do a most abundant search because it will be comparing the scan most abundant and the peptide monoisotopic.

I am not sure what the solution is, but this has potential to cause a lot of bugs and confusion. Maybe something like having mono and avg mass as separate fields then the code that needs it will pick the correct one? Maybe a helper method that is GetPrecursorMass(bool isMostAbundant) in both ms2scanwithspecificmass and SpectralMatch. Maybe it's an extension method instead?

Comment thread MetaMorpheus/EngineLayer/Calibration/DataPointAquisitionResults.cs Outdated
Comment thread MetaMorpheus/EngineLayer/Ms2ScanWithSpecificMass.cs Outdated
Comment thread MetaMorpheus/EngineLayer/SpectralMatch.cs Outdated
trishorts and others added 3 commits July 13, 2026 21:22
…ptor pick the search mass

Per nbollis's review: Ms2ScanWithSpecificMass and SpectralMatch must not change
meaning with the search type being run. PrecursorMassToMatch did exactly that —
it was the monoisotopic mass in one search and the most-abundant mass in another,
so any call site reading PrecursorMass could be silently wrong in most-abundant
mode, and the two properties were easy to confuse.

Both classes now carry only OBSERVATIONS, populated identically in every search:

  PrecursorMass             the deconvoluted monoisotopic mass, always
  PrecursorMostAbundantMass the observed apex of the envelope (0 if none)

PrecursorMassToMatch / ScanPrecursorMassToMatch are gone. PrecursorMass keeps its
original meaning everywhere, so no existing call site can be caught out by it.

The knowledge of WHICH observation a search matches on now lives on the object
that defines the search type — the MassDiffAcceptor (MatchesMostAbundantPeak) —
and is reached through an extension method, as nbollis suggested:

  scan.GetPrecursorMassForSearch(massDiffAcceptor)
  psm.GetPrecursorMassForSearch(massDiffAcceptor)

Classic/Modern search and the three task-level scan sorts all go through it, so
the sort key and the search key cannot drift apart. GetMs2Scans no longer branches
on the search mode at all: it records the apex whenever deconvolution produced one,
because that is an observation, not a decision.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… read as chemistry

nbollis flagged that the localization engine would misbehave in most-abundant
mode. He was right, though by a different mechanism than the one he described:
ScanPrecursorMass stays monoisotopic, so localization is comparing monoisotopic
to monoisotopic — but most-abundant matching deliberately ACCEPTS candidates whose
deconvoluted monoisotopic peak is off by 1-2 neutrons (that is what the apex notch
set is for, and those PSMs are precisely the ones this mode exists to rescue). For
them, (ScanPrecursorMass - peptideMonoisotopicMass) is a whole-isotopologue offset,
not chemistry. Three consumers were reading it as chemistry:

  LocalizationEngine  localized a phantom ~1-2 Da modification onto a residue
  GptmdEngine         fed the same bogus difference into mod-candidate matching
  DataPointAquisition calibration counted it as m/z drift (the 1940 ppm runaway)

Only calibration had been patched, in-place and with a hardcoded C13MinusC12 —
the constant nbollis also flagged in this review.

All three now go through MassDiffAcceptor.ToObservedMonoisotopicMass(), which
undoes exactly the offsets the acceptor itself allowed (the averagine apex offset
and the k-neutron apex misprediction), using the isotope spacing it was given from
the deconvolution parameters. No hardcoded neutron mass remains in these paths.
For every monoisotopic acceptor the method is the identity, so default-mode
behaviour is unchanged, bit for bit.

The Dot acceptor re-derives the (shift, k) pair rather than reading k back from the
notch, because its notch identifies only the mass shift — all k for a shift share
one notch to preserve per-shift FDR grouping. Its class remark claimed G-PTM-D was
"unaffected by this acceptor"; that claim was the bug, and it is corrected.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…otope-offset fix

- Ms2Scan_PrecursorMasses_AreObservations_AcceptorSelectsBetweenThem: both masses
  are recorded in every search; the acceptor alone decides which is matched; with
  no observed apex, even a most-abundant acceptor selects the monoisotopic mass.
- ToObservedMonoisotopicMass_RemovesApexAndNeutronOffsets (k = -2..+2): an accepted
  match with a k-neutron apex misprediction yields a ~0 mass difference instead of
  k * 1.00335 Da, so localization/G-PTM-D cannot see a phantom modification. A
  monoisotopic acceptor is asserted to be the identity.
- GetMs2Scans_RecordsApexObservation_RegardlessOfSearchMode: the scan no longer
  changes shape with the search mode.

CalibrationPrecursorError_StripsIsotopeOffsets_InMostAbundantMode replaces the old
neutron-stripping test, which never exercised the real path: it built PSMs with no
observed apex at all, so the correction it asserted was running on a fallback. It
now builds a real apex observation plus a k-neutron misprediction, and adds a
control asserting the same PSMs show a >50 ppm IQR when read without the acceptor —
proving the correction, not the fixture, is what tightens the spread.

Full suite: 1550 passed, 0 failed.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@trishorts trishorts dismissed stale reviews from Alexander-Sol and RayMSMS via a9b0449 July 14, 2026 04:49
@trishorts

Copy link
Copy Markdown
Contributor Author

This was the right call, and it was worse than you thought. Restructured across three commits (4102843, 0e4ba37, a9b0449) — full suite green, 1568/1568.

1. The representations are now stateless w.r.t. search type

PrecursorMassToMatch / ScanPrecursorMassToMatch are deleted. Ms2ScanWithSpecificMass and SpectralMatch carry only observations, populated identically in every search:

property meaning
PrecursorMass the deconvoluted monoisotopic mass — always, in every mode
PrecursorMostAbundantMass the observed apex of the envelope (0 if none was deconvoluted)

PrecursorMass never changes meaning, so it can't be silently wrong. The search-type knowledge lives on the MassDiffAcceptor — the object that is the search type — and is reached by extension method, your suggestion #3:

scan.GetPrecursorMassForSearch(massDiffAcceptor)
psm.GetPrecursorMassForSearch(massDiffAcceptor)

GetMs2Scans no longer branches on the mode at all. Classic/Modern search and all three scan sorts go through the extension, so the sort key and the search key can't diverge.

2. Your localization example — inverted mechanism, real bug

Strictly, the mechanism you describe doesn't occur: ScanPrecursorMass stays monoisotopic in most-abundant mode, so LocalizationEngine is comparing scan-monoisotopic to peptide-monoisotopic.

But the conclusion is correct, for a sharper reason. Most-abundant matching deliberately accepts candidates whose deconvoluted monoisotopic peak is off by ±1–2 neutrons — that's the entire point of the apex notch set, and those PSMs are exactly the ones this feature exists to rescue. For them, ScanPrecursorMass - peptide.MonoisotopicMass ≈ k · 1.00335 Da. That is an isotope-assignment artifact, not chemistry — and three places were reading it as chemistry:

  • LocalizationEngine:58 — localized a phantom ~1–2 Da modification onto a residue.
  • GptmdEngine:104 — fed the same bogus difference into mod-candidate matching. This one is a bug we shipped in this PR, and you found it. Worse, MostAbundantDotMassDiffAcceptor carried a remark asserting the opposite — "downstream G-PTM-D mod assignment recomputes the modification from the monoisotopic precursor mass, so it is unaffected by this acceptor." That claim was the bug. Corrected in code and comment.
  • DataPointAquisitionResults — counted it as m/z drift (the 1940 ppm runaway tolerance). This was the only one we'd found, and we'd patched it in place with the hardcoded C13MinusC12 you flagged above — i.e. we hit this bug class once, fixed the symptom, and missed that it was systemic. Your review is what surfaced that.

All three now go through one accessor:

public virtual double MassDiffAcceptor.ToObservedMonoisotopicMass(double observedPrecursorMass, double theoreticalMonoisotopicMass)

which undoes exactly the offsets the acceptor itself allowed (averagine apex offset + the k-neutron apex misprediction), using the isotope spacing it took from DeconvolutionParameters. It is the identity on every monoisotopic acceptor, so default-mode behaviour is unchanged bit-for-bit — the calibration/localization/G-PTM-D diffs are all "route through the acceptor," not "special-case most-abundant."

One design note: I did not use GetPrecursorMass(bool isMostAbundant). A bool at the call site is the same coupling in a thinner disguise — the caller still has to know which search it's in. Asking the acceptor means there's exactly one source of truth. It also turned out to be load-bearing for G-PTM-D: the Dot acceptor's notch encodes only the mass shift (all k share a notch, to preserve per-shift FDR grouping), so k can't be read back from the notch — the acceptor has to re-derive the (shift, k) pair itself, and only the acceptor knows its shift set.

3. Tests

New coverage for the observation/selection split, for ToObservedMonoisotopicMass across k = −2..+2 (asserting the mass difference collapses to ~0 rather than k·1.00335, and that a monoisotopic acceptor is the identity), and for the scan no longer changing shape with the mode.

The old calibration test was also replaced: it never exercised the path it claimed to — it built PSMs with no observed apex, so the "neutron stripping" it asserted was running on a fallback. It now builds a real apex observation plus a k-neutron misprediction, with a control asserting the same PSMs show a >50 ppm IQR when read without the acceptor, so the test proves the correction (not the fixture) is what tightens the spread.

@nbollis nbollis left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Love the direction this is going in. I beleive in most cases where the mass diff acceptor was added to the engine parameters (for the purpose of switching masses) we only need the enum in common params which is passed into all engines. Therefor if we make the extension methods operate upon the enum, we will not need the mass diff acceptor in the engines.

In addition, you should do a search for all .PrecurorMasses in the codebase for both Ms2scanWithSpecificMass as well as SpectralMatch. There are likely numerous locations to make the switch yet.

/// these extensions are the one place that asks it.
/// </para>
/// </summary>
public static class PrecursorMassExtensions

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be great if these could operate upon booleans and/or the new enum you created that is stored in common parameters. I think this would allow you to get away with not adding the mass diff accepter to all of the engines.

You could then pass the common params enum into the extension method to get the correct mass

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done — the extensions now take CommonParameters and read PrecursorMassMatchMode (commit fe221b0).

You were right that this pays for itself: MassDiffAcceptor is back to untouched (the MatchesMostAbundantPeak and ToObservedMonoisotopicMass virtuals I added are deleted), and the acceptor parameter is gone from DataPointAcquisitionEngine, LocalizationEngine and GptmdEngine — all three already had CommonParameters, so those signatures return to exactly what they were.

Three methods, and which one you want depends on the question you're asking:

scan.GetPrecursorMassForSearch(commonParameters)       // which mass do we SELECT candidates with
psm.GetObservedMonoisotopicMass(theoretical, params)   // read a mass DIFFERENCE without the isotope offset
psm.MatchesCandidateMass(candidate, tol, params)       // does the observation support this candidate

One place the enum alone genuinely isn't enough, and I want you to sanity-check my reasoning: G-PTM-D.

GetObservedMonoisotopicMass recovers the k-neutron apex misprediction by rounding the residual to the nearest isotopologue. That is exact for calibration and localization because most-abundant mode always searches with the zero-centred apex acceptor — GetMassDiffAcceptor overrides the MassDiffAcceptorType in that mode — so the residual is k neutrons plus a within-tolerance ppm error and nothing else.

G-PTM-D breaks that invariant: it searches with discrete mass shifts, so the residual also contains an unknown modification mass. Round it and the modification gets folded into k and destroyed (a phospho at +79.966 would come back as ~-0.06 Da). The mod mass and the isotope offset land in the same residual and cannot be separated after the fact.

So G-PTM-D doesn't correct the precursor once — GetPossibleMods tests each candidate mass (peptide + modification) against the observation apex-to-apex via MatchesCandidateMass. That's the same acceptance rule as MostAbundantDotMassDiffAcceptor.Accepts, just expressed over the deconvolution parameters, so it still needs no acceptor. Its combo recursion now accumulates a candidate-mass offset rather than subtracting from the target mass — equivalent arithmetic, but it leaves the observed mass untouched so the apex test always sees what was actually measured.

New test pins it: a phosphorylated proteoform observed one neutron off its apex is still matched to phospho, and still not matched to oxidation.

MostAbundantMassDiffAcceptor.DefaultMaxApexOffsetNeutrons is now shared with the extensions, so the acceptor and the read-back can't drift apart on the k range.

trishorts and others added 2 commits July 14, 2026 15:15
…ameters, not the acceptor

Per nbollis (review round 2): the extension methods now take CommonParameters and
read PrecursorMassMatchMode, so the MassDiffAcceptor no longer has to be threaded
into engines that never needed it for anything else.

MassDiffAcceptor is back to untouched — MatchesMostAbundantPeak and
ToObservedMonoisotopicMass are deleted — and the acceptor parameter is removed from
DataPointAcquisitionEngine, LocalizationEngine and GptmdEngine. All three already
had CommonParameters, so their signatures return to what they were.

  scan.GetPrecursorMassForSearch(commonParameters)      which mass do we SELECT with
  psm.GetObservedMonoisotopicMass(theoretical, params)  read a mass DIFFERENCE safely
  psm.MatchesCandidateMass(candidate, tol, params)      does the observation support this candidate

G-PTM-D is the one place the enum alone does not suffice. Calibration and localization
can recover the k-neutron apex misprediction by rounding, because most-abundant mode
always searches with the zero-centred apex acceptor (GetMassDiffAcceptor overrides
MassDiffAcceptorType), so the residual is k neutrons plus a within-tolerance ppm error
and nothing else. G-PTM-D searches with discrete mass shifts, so the residual also
contains an UNKNOWN modification mass — rounding would fold the modification into k and
destroy it. GetPossibleMods therefore tests each candidate mass (peptide + modification)
against the observation apex-to-apex via MatchesCandidateMass: the same acceptance rule
as MostAbundantDotMassDiffAcceptor.Accepts, expressed over the deconvolution parameters,
so no acceptor is needed. Its combo recursion now accumulates a candidate-mass offset
instead of subtracting from the target mass — equivalent arithmetic, but it leaves the
OBSERVED mass untouched so the apex test always sees what was really measured.

Also, per nbollis's second ask, an audit of every Ms2ScanWithSpecificMass.PrecursorMass
and SpectralMatch.ScanPrecursorMass consumer. Three more sites were reading the wrong
mass and are switched:

  NonSpecificEnzymeSearchEngine  selected candidates off the raw monoisotopic mass in two
                                 places, so non-specific search silently ignored the mode
  BinTreeStructure               built the mass-shift histogram from the raw difference —
                                 the localization defect again, which would have grown
                                 spurious ~1-2 Da bins that look like real modifications
  PSM ranking tie-breakers       (CalibrationTask, NonSpecificEnzymeSearchEngine x2) rank
                                 equal-scoring matches by precursor closeness; the isotope
                                 offset is not "closeness", and left in it would rank a
                                 correct match with a 1-neutron apex misprediction behind
                                 an inferior one

Deliberately left on the monoisotopic mass: fragment-bin and complementary-ion math
(ModernSearchEngine, MetaMorpheusEngine), where the monoisotopic mass is what the fragment
math means; all reporting and writing (PsmTsvWriter, PepXMLWriter, WriteXlFile, MetaDraw),
so the psmtsv Mass Diff column stays comparable across modes with the most-abundant column
reported alongside it; and crosslink/glyco search, which do not expose this mode.

MostAbundantMassDiffAcceptor.DefaultMaxApexOffsetNeutrons is now shared with the
extensions, so the acceptor and the read-back cannot disagree about the k range.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…G-PTM-D case

- Ms2Scan_PrecursorMasses_AreObservations_SearchModeSelectsBetweenThem and
  GetMs2Scans_RecordsApexObservation_RegardlessOfSearchMode now select via the search
  mode rather than an acceptor.
- GetObservedMonoisotopicMass_RemovesApexAndNeutronOffsets (k = -2..+2) builds a real PSM
  whose deconvoluted monoisotopic peak is off by k neutrons, asserts the corrected mass
  difference collapses to the ~2 ppm instrument drift, and asserts that read RAW the same
  PSM shows a whole-neutron difference — the phantom modification, pinned explicitly.
- MatchesCandidateMass_FindsModification_DespiteApexMisprediction (new): a phosphorylated
  proteoform observed one neutron off its apex is still matched to phospho, and is still
  NOT matched to oxidation. This is the case a rounding correction cannot handle, and is
  why G-PTM-D tests candidates individually.

Full suite: 1569 passed, 0 failed.

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

Copy link
Copy Markdown
Contributor Author

Both asks done — fe221b0 (production) and 92e13e0 (tests). Full suite green, 1569/1569.

1. Extensions operate on the enum; acceptor out of the engines

MassDiffAcceptor is back to untouched, and the acceptor parameter is removed from DataPointAcquisitionEngine, LocalizationEngine and GptmdEngine. Details and the one exception (G-PTM-D, which searches with discrete mass shifts, so the isotope offset can't be recovered by rounding) are in the thread reply above — worth a look, since that's the only place I didn't do exactly what you asked.

2. The audit — you were right, there were more

Swept every Ms2ScanWithSpecificMass.PrecursorMass and SpectralMatch.ScanPrecursorMass consumer. Three more sites were reading the wrong mass:

site what was wrong
NonSpecificEnzymeSearchEngine:88, :128 Selected candidates off the raw monoisotopic mass — so non-specific search silently ignored most-abundant mode entirely.
BinTreeStructure:19, :97 Builds the mass-shift histogram from ScanPrecursorMass - peptideMonoisotopic. Same defect as localization: it would have grown spurious ~1 and ~2 Da bins that look like real modifications.
PSM ranking tie-breakers (CalibrationTask, NonSpecificEnzymeSearchEngine ×2) Rank equal-scoring matches by precursor closeness. An isotope offset is not "closeness" — left in, a correct match with a 1-neutron apex misprediction ranks behind an inferior one.

Deliberately left on the monoisotopic mass — flagging these explicitly so you can push back rather than have it be silent:

  • Fragment-bin / complementary-ion math (ModernSearchEngine, MetaMorpheusEngine.MatchFragmentIons). The monoisotopic mass is what that math means; the apex mass would be wrong there.
  • All reporting and writing (PsmTsvWriter, PepXMLWriter, WriteXlFile, MetaDraw). The psmtsv Mass Diff column stays monoisotopic-vs-monoisotopic so it remains comparable across search modes, with the optional Most Abundant Mass Diff (ppm) column reported alongside it. Read together they separate the two error sources.
  • Crosslink / glyco search. They build their own acceptors and don't expose this mode. Worth noting: if a user sets PrecursorMassMatchMode = MostAbundant in a toml and runs an XL or glyco task, it is currently a silent no-op. Happy to make that an explicit warning (or wire it up) if you'd rather — say the word and I'll do it in this PR.

3. Tests

MatchesCandidateMass_FindsModification_DespiteApexMisprediction is the new one worth reading — it's the case a rounding correction cannot handle. GetObservedMonoisotopicMass_RemovesApexAndNeutronOffsets (k = −2..+2) now also asserts that read raw, the same PSM shows a whole-neutron mass difference, so the phantom modification is pinned explicitly rather than just being absent.

@nbollis nbollis left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One question and one suggestion

/// only the deconvolution parameters, not the acceptor's shift set.
/// </para>
/// </summary>
public static bool MatchesCandidateMass(this SpectralMatch psm, double candidateMonoisotopicMass, Tolerance tolerance, CommonParameters commonParameters)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this method needed if we have the special mass diff accepters?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair question — they do compute the same test, so let me justify why the method exists rather than the acceptor.

MatchesCandidateMass and MostAbundantDotMassDiffAcceptor.Accepts are the same apex-to-apex arithmetic (candidate apex = mono + averagine offset; accept if the observed apex is within tolerance of apex + k·spacing for k in ±DefaultMaxApexOffsetNeutrons). It is deliberately not a second implementation — both go through the same AveragineApexOffset model and share DefaultMaxApexOffsetNeutrons.

The reason it's a method on the extensions and not just a call to .Accepts is your own round-2 request: take the MassDiffAcceptor back out of the engines and drive everything off CommonParameters. GptmdEngine.GetPossibleMods is inside that boundary. So the choice was:

  1. thread the Dot acceptor back into GetPossibleMods purely so it can call .Accepts — which puts back exactly the coupling you asked me to remove, in the one engine where I couldn't use the rounding correction, or
  2. express the same acceptance test over CommonParameters — which is MatchesCandidateMass.

I went with (2) so no engine holds an acceptor. If you'd rather GPTMD just take the MostAbundantDotMassDiffAcceptor and call .Accepts directly, I'm happy to switch — it's a wash on correctness, and it would delete MatchesCandidateMass. The tradeoff is purely "GPTMD carries an acceptor again" vs. "the apex test lives in one more small method." Your call; tell me which you prefer.

(One structural note in favour of keeping it off the acceptor: Accepts folds the candidate's shifts into its own shift set, whereas GetPossibleMods already enumerates each fully-formed candidate mass and just needs a yes/no on it. MatchesCandidateMass fits that caller exactly; routing through Accepts would mean constructing a Dot acceptor whose shift set is the single candidate, which is a stranger shape than the plain per-candidate test.)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess I am confused why GPTMD specifically needed this correction but it is not needed elsewhere

internal static void AddPeptideSequenceData(Dictionary<string, string> s, SpectralMatch sm, IReadOnlyDictionary<string, int> ModsToWritePruned)
internal static void AddPeptideSequenceData(Dictionary<string, string> s, SpectralMatch sm, IReadOnlyDictionary<string, int> ModsToWritePruned, bool includeMostAbundantColumn = false)
{
bool pepWithModsIsNull = sm == null || sm.BestMatchingBioPolymersWithSetMods == null || !sm.BestMatchingBioPolymersWithSetMods.Any();

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should likely also write out the MostAbundantMass as a new column in addition to the mass error, but I will not hold up the PR on this

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed, and I've teed up the mzLib half so we can do it right (owned by mzLib, not MM-local):

smith-chem-wisc/mzLib#1097 — adds the optional Precursor Most Abundant Mass psmtsv column to the reader/header. Same recipe as #1086 (which added Most Abundant Mass Diff (ppm)): optional, mapped by name, reads back null when absent, so old files are unaffected. The one difference from the error column is that this is an observation of the precursor envelope, not a per-hypothesis value — so it's one number per row and copies straight through disambiguation rather than being |-split.

Once #1097 releases, the MetaMorpheus side is the same pattern we used for the error column: PsmTsvWriter writes SpectralMatch.ScanPrecursorMostAbundantMass keyed by the mzLib header const, gated by the same includeMostAbundantColumn flag so normal output is unchanged. Since you said you won't hold up this PR on it, I'll add the MM writer change here once #1097 is merged/released, or as an immediate follow-up PR — whichever you prefer for merge sequencing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants