Repository navigation
Conversation
Every peptide winnow published had its leucines rewritten to isoleucine. Measured over 20,000 human spectra: instanovo's predictions going in were 30.4% isoleucine, the ground-truth labels 34.2%, and the published prediction and sequence columns 100% -- not one leucine survived either. The collapse belongs to comparison, where the two residues being isobaric means a prediction differing only in I/L is still correct. Applying it to the columns themselves put a residue in the output that was never predicted: ALAHKYH was published as AIAHKYH, no longer matched the protein it came from, and was reported downstream as a peptide needing an I/L substitution to map. The model had it right. normalize_peptide_cell now canonicalises modification notation only, and leaves residues as they were given. The collapse moves to the two row evaluation functions, which is the only place a prediction is compared against a label, so num_matches and correct are unchanged. _normalize_leucine_tokens becomes replace_isoleucine_with_leucine: named for the residue that disappears, because the direction is the whole content of the operation and both directions are in use across these repositories. It now follows instanovo and the peptide registry, which collapse I into L. Direction does not affect any comparison here, since both sides are collapsed the same way. Beam features keep the collapse. Those sequences are compared against each other and never reported. Seven tests asserted the old behaviour and now assert the new one. The shared loader fixture gained "L", which it had never needed while everything was being rewritten to "I"; the production residues.yaml has defined both all along. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01C8sMwNQJTyWwJB5Qn7vjuW
Checked against the paper reproduction scripts (#240)#240 adds 28 scripts under No API breakage. Seven scripts already collapse I→L on their own side before comparing — The one script comparing the columns directly is mass-based. One real consequence: Noted on #240 as well. |
|
This is a valuable addition to Winnow, but I have some issues with the design of the fix.
|
| ) | ||
| sequence_valid = bool(row["valid_sequence"]) | ||
| prediction_valid = bool(row["valid_prediction"]) | ||
| num_matches = row_num_matches( |
There was a problem hiding this comment.
I would put the I/L remapping inside row_num_matches because that metric is really why we are doing it, so imo they should always go hand-in-hand.
| "T": 101.047670, | ||
| "C": 103.009185, | ||
| "I": 113.084064, | ||
| "L": 113.084064, # isobaric with I; the production config defines both |
| """Beam sequences are compared against each other, so I is collapsed into L. | ||
|
|
||
| These are feature inputs, never reported, which is why collapsing them is | ||
| safe. The direction follows InstaNovo and the peptide registry. |
There was a problem hiding this comment.
This comment is a bit vague, what is "the peptide registry" to a future contributor? I am also not sure beams are never reported, they can be saved as part of a CalibrationDataset. See my general comment on the PR, but continuing to always collapse only the InstaNovo beams is not safe.
|
|
||
| This column is reported. Collapsing I/L here put a residue in the output that | ||
| was never predicted, and a peptide correct as written then failed to match a | ||
| reference spelling it the other way. |
There was a problem hiding this comment.
This comment implies that Winnow had an internal bug with its own peptide correctness metric, which is not the case. Winnow always judges a peptide to be correct treating leucine and isoleucine as equivalent. The comment likely comes from an error experienced downstream of Winnow's use.
| self, metrics: Metrics | ||
| ) -> None: | ||
| """Uppercase L inside a modification name must not be rewritten before split.""" | ||
| """Uppercase L inside a modification name must survive, as must the residue.""" |
There was a problem hiding this comment.
Nit but I find this personification too flowery and generally prefer more technical terminology
| despite the two residues being isobaric. Applying it to a reported column | ||
| rewrote predictions the model had right -- `ALAHKYH` was published as | ||
| `AIAHKYH`, stopped matching the protein it came from, and was reported as | ||
| needing an I/L substitution. |
There was a problem hiding this comment.
Another comment that is bringing in unneeded context from outside of Winnow's purview. I would cut the whole thing. The test only needs a single sentence docstring, and even then only if the name does not sufficiently cover its purpose. To that end, I think this method could be named more clearly for its purpose.
| reported = data_utils.normalize_peptide_cell( | ||
| predicted, metrics, residue_remapping=REMAPPING | ||
| ) | ||
| assert reported == predicted |
There was a problem hiding this comment.
Why is this being tested in here?
| def _normalize_leucine_tokens(tokens: list[str]) -> list[str]: | ||
| """Map leucine to isoleucine at the token level.""" | ||
| return ["I" if token == "L" else token for token in tokens] | ||
| def replace_isoleucine_with_leucine(tokens: list[str]) -> list[str]: |
There was a problem hiding this comment.
This was a good rename ty
| def replace_isoleucine_with_leucine(tokens: list[str]) -> list[str]: | ||
| """Rewrite every isoleucine token as leucine. | ||
|
|
||
| Named for the residue that disappears, because the direction is the whole content |
There was a problem hiding this comment.
Way too much docstring bloat for such a short method. The first sentence says it all, just add Google-style args and returns since it's public now.
| ) -> tuple[int, bool]: | ||
| sequence = as_token_list(row.get(sequence_col)) or [] | ||
| prediction = as_token_list(row.get(prediction_col)) or [] | ||
| # Collapsed here and nowhere else: isoleucine and leucine are isobaric, so a |
There was a problem hiding this comment.
See comment below about placement of the remapping inside the matching method. This would remove the need for these duplicated comments here (just put in the docstring).
|
Closed in favour of #248 |
2 participants
Every peptide winnow publishes has its leucines rewritten to isoleucine. Measured over 20,000 human spectra:
predictionandsequencecolumnsNot one leucine survived either column.
Why that is a bug rather than a convention
Collapsing I and L belongs to comparison. The two residues are isobaric, so a prediction differing from its label only in I/L is still correct, and scoring should say so. Applying the collapse to the columns themselves is a different thing: it puts a residue in the output that was never predicted.
The downstream consequence is not cosmetic.
ALAHKYHwas published asAIAHKYH, which no longer matches the protein it came from, so the peptide-to-protein step reported it as needing an I/L substitution to map — a substitution the model never made. In a pipeline that reports a mass-equivalent misread as a novel peptide, that is the most consequential direction to be wrong in. On the same 20,000 spectra it put 76% of mapped peptides into an I/L-dependent tier; with this fix the figure is 8.3%, and exact matches go from 662 to 2,932.It also collapses in the opposite direction to everything around it. InstaNovo and the peptide registry rewrite I into L; winnow rewrote L into I. A key built under one convention matches nothing under the other, and the failure mode is an empty result rather than a wrong one.
What changes
normalize_peptide_cellcanonicalises modification notation only and leaves residues as they were given.The collapse moves to the two row-evaluation functions, which are the only places a prediction is compared against a label.
num_matchesandcorrectare therefore unchanged — verified by the existing tests, which pass untouched where they assert on scoring._normalize_leucine_tokensbecomesreplace_isoleucine_with_leucine, named for the residue that disappears. The direction is the whole content of the operation and both directions are in use across these repositories, so a name that does not state it is a trap. It now follows InstaNovo and the registry by collapsing I into L; direction does not affect any comparison here, since both sides are collapsed the same way.Beam features keep the collapse.
_beam_edit_distancecompares two beam sequences against each other and never reports them, so it is comparison and stays.The
normalize_leucineparameter of_apply_token_postprocessinggoes, since there is no longer a caller that wants residues rewritten on the way in.Tests
Seven tests asserted the old behaviour and now assert the new one — that a published column keeps the spelling it was given, and that scoring still treats I and L as equal.
Context
Found while tracing a figure that could not be reconciled: the pipeline reported that 76% of mapped peptides needed an I/L substitution, which is far more than InstaNovo's measured I/L accuracy predicts (92% balanced accuracy over 21,829 forced choices on labelled data). The discrepancy was this rewrite.
Downstream issue: instadeepai/INFlow#132. The same audit found sibling faults in InstaNovo's mapping loader and in InstaNexus's assembled scaffolds, both fixed separately.