fix(features): correct positional modification indexing, and optional terminal composition - #112
Merged
Merged
Conversation
_fill_pos_matrix lays out the positional block as sorted(positions), so row 0
means position -4 and row 4 means position 0. Modification deltas were written
with raw indexing instead, so every positional delta landed on the wrong row:
an N-terminal delta was recorded four residues from the C-terminus.
a = encode_peptidoform("PEPTIDEK")
b = encode_peptidoform("[Acetyl]-PEPTIDEK")
# before: the delta appears in row 0, meaning position -4
Base residues are also written to both a positive and a negative row when a
short peptide makes one residue occupy both (index 3 of a 7-mer is also -4),
because the two loops in _fill_pos_matrix run independently. Deltas used
if/elif and reached only one row. Both are now resolved through
_positional_rows, so deltas follow the base residues exactly.
Also adds an opt-in add_terminal_composition flag appending the N- and
C-terminal group compositions to matrix_global (55 -> 67 values). Terminal
groups are folded into matrix and the positional block, but there they cannot
be told apart from a modification on the side chain of the first or last
residue: [Acetyl]-PEPTIDEK and P[Acetyl]EPTIDEK produced identical features.
The flag defaults to False so existing models keep their feature width.
Verified against 1,200 peptidoforms encoded independently: matrix_global now
matches on 1,199.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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.
Why
Two problems in
_features.py, both verified against currentmain.1. Positional modification deltas land on the wrong row (bug)
_fill_pos_matrixlays the positional block out assorted(positions), so row 0 means position-4and row 4 means position0. Modification deltas were written with raw indexing instead:Every positional modification delta was therefore recorded at the opposite end of the peptide from the residue it belongs to:
There is a second, subtler half. In a short peptide one residue occupies both a positive and a negative row (index 3 of a 7-mer is also
-4)._fill_pos_matrixwrites the base residue to both, because its two loops run independently, but the delta path usedif/elifand reached only one.MAECLIR|4|Carbamidomethylis an example.Both are now resolved through a single
_positional_rowshelper, so deltas follow the base residues exactly.2. Terminal and side-chain modifications are indistinguishable (gap)
_apply_terminal_modificationscorrectly folds terminal groups intomatrixand the positional block, but at the same position as a side-chain modification on the first or last residue, so the two produce identical features:They are chemically different and elute differently.
Implementation
_positional_rows(i, seq_len, positions)returns every row a sequence position occupies, applying the same offset and both-ends handling as_fill_pos_matrix._apply_composition_to_matriceswrites through it.encode_peptidoform(..., add_terminal_composition=True)appends the N- and C-terminal group compositions, takingmatrix_globalfrom 55 to 67 values. Defaults toFalse, so existing models keep their feature width and behaviour is unchanged apart from the bug fix.How to test
Two regression tests are added:
test_positional_delta_uses_same_row_as_base_residueasserts an N-terminal delta lands on the position-0 row and a C-terminal delta on the position -1 row.test_terminal_composition_is_opt_in_and_separates_terminal_from_side_chainasserts the default width stays 55, the flag gives 67, and the two acetyl forms differ.All 68 tests pass. Separately, 1,200 peptidoforms were encoded through this code and through an independent implementation:
matrix_globalmatches on 1,199 of 1,200 after the fix, against 3 of 400 differing before.Impact
The fix changes features for every peptidoform carrying a modification at a tracked position. Models trained against the previous behaviour will need retraining or revalidation. The shipped models learned around the misplaced deltas to some degree, but corrected features are what a new model should use.
Follow-up, not in this PR
A multitask model trained on the corrected 67-value features reaches 0.540 min pooled MAE across 1,025 LC setups (2.36M held-out observations, median absolute error 0.220, r 0.9987).
That model is being retrained against this branch's extractor. Its feature cache was built by the 2.x extractor, so it saw terminal composition in
matrix_globalbut not inmatrix, whereas this branch supplies both; serving it as-is would give it features it was not trained on for the ~0.5% of peptidoforms carrying a terminal modification. It will follow as a separate PR once validated.Calibration is deliberately left unchanged. Replacing best-head selection with a ridge over head outputs improves median relative MAE from 2.49% to 2.31% across 20 datasets at 1,000 calibration peptides and wins on 17 of 20 — but it is a wash at 200 peptides, worse on several runs, and the larger gains measured elsewhere depend on a low-rank linear read-out rather than these ReLU heads. Not a good enough trade to change a working default, so best-head selection stays.
🤖 Generated with Claude Code