Repository navigation
Keep both modifications that share a residue on generated decoy fragments - #71
Merged
Merged
Conversation
…ents The library loaders put an N-terminal modification on residue 0, where a modification of the first residue also sits ((UniMod:1)M(UniMod:35)), and the target's fragments carry both. recalculate_fragments built its per-position mass map with insert, so the second modification replaced the first and every decoy fragment spanning that residue was one modification light - 42 Da for an N-terminal acetyl. The masses at one position now add. Only libraries holding such an entry change; none of the regression libraries does (Stellar, Astral and stellar-libdecoy scanned). Mirrors ProteoWizard/pwiz (DecoyGenerator.RecalculateFragments, which now sums through PeptideFragmentMass.ModMassesByPosition); test_decoy_fragment_carries_stacked_modifications is the Rust twin of DecoyConstructionTest.DecoyFragmentCarriesStackedModifications, and fails before the fix on b8. Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
Copilot review overview
🟢 Approval recommended
The focused fix is correct and covered by a representative regression test.
Review effort: Balanced
Findings: None
What changed in this PR
Fixes generated decoy fragment masses when multiple modifications share one residue.
Changes:
- Accumulates modification mass deltas per decoy position.
- Adds regression coverage for stacked N-terminal/residue modifications.
- Documents the fix in release notes.
| File | Description |
|---|---|
crates/osprey-scoring/src/lib.rs |
Sums co-located modifications and tests affected fragments. |
release-notes/RELEASE_NOTES_next.md |
Records the decoy mass correction. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| // Find new position for this modification | ||
| if let Some(new_pos) = position_mapping.iter().position(|&old| old == m.position) { | ||
| mod_masses.insert(new_pos, m.mass_delta); | ||
| *mod_masses.entry(new_pos).or_insert(0.0) += m.mass_delta; |
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.
Summary
(UniMod:1)M(UniMod:35)), and the target's fragments carry both masses.recalculate_fragmentsbuilt its per-position mass map withinsert, so the second modification replaced the first. Every decoy fragment spanning that residue came out one modification light: 42.011 Da for an N-terminal acetyl.*mod_masses.entry(new_pos).or_insert(0.0) += m.mass_delta). With at most one modification per position the map is unchanged, so only libraries that hold such an entry get different decoys. None of the regression libraries does: Stellar, Astral and stellar-libdecoy were scanned.DecoyGenerator.RecalculateFragmentsnow sums throughPeptideFragmentMass.ModMassesByPosition, so the two implementations keep generating the same decoys.Test plan
cargo fmt --check,cargo clippy --workspace -- -D warnings,cargo test --workspacetest_decoy_fragment_carries_stacked_modifications(targetMPEPTIDEKwith acetyl and oxidation on residue 0; decoyEDITPEPMK): b3 carries neither mass, while b8, y2 and y4 carry both. It failed before the fix on b8.DecoyConstructionTest.DecoyFragmentCarriesStackedModificationsfails the same way before its fix, and the C# Stellar regression passes unchanged.Co-Authored-By: Claude noreply@anthropic.com