Fix AA-identity/property-value mismatch in sequenceEncoder()/sequenceDecoder() property mode - #22
Merged
ncborcherding merged 6 commits intoAug 22, 2026
Conversation
.aa.property.matrix() returned property scales in whatever column order Peptides::AAdata happened to store them in (alphabetical for MSWHIM/ProtFP, E/Q-swapped for crucianiProperties), not the canonical amino-acid order that sequenceEncoder()'s C++ backend and sequenceDecoder()'s distance-based caller both assume positionally. calculateProperty() already reordered by name after calling this helper; sequenceEncoder()/sequenceDecoder() did not, so e.g. encoding "R" under MSWHIM silently returned Cysteine's values. Centralizes the canonical-AA reorder inside .aa.property.matrix() itself so every caller gets it by construction, fixing both call sites at once. Verified against Peptides' own independent public scoring functions (mswhimScores(), crucianiProperties(), protFP()), not just immApex internals. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Contributor
Author
|
@ncborcherding Note: the test-coverage fail related to |
Correcting documentation
Updating using the use of sequence.dictionary
Codecov Report❌ Patch coverage is
🚀 New features to boost your workflow:
|
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.
Bug
sequenceEncoder(mode="property", ...)andsequenceDecoder(mode="property", ...)silently mismatch amino-acid identity to property values for 3 of the ~10
supported property sets:
crucianiProperties,MSWHIM,ProtFP.Both functions assume
.aa.property.matrix()'s output is ordered by thecanonical amino-acid alphabet, positionally —
sequenceEncoder()'s roxygendocs state this explicitly ("rows correspond to the 20 canonical amino acids
in the order of
sequence.dictionary"), andsequenceEncoder()'s C++backend /
sequenceDecoder()'s.propertyDecoder()both index into itpurely positionally, never by name.
.aa.property.matrix()didn't actually guarantee that. It returnsPeptides::AAdata[[key]]reshaped viado.call(rbind, ...), in whatevercolumn order that scale's author originally stored it in Peptides — which is
NOT canonical for these 3 sets:
crucianiPropertiesE,Qinstead ofQ,E)MSWHIMProtFPcalculateProperty()already reorders by name after calling this helper(
S[, amino.acids, drop=FALSE]);sequenceEncoder()/sequenceDecoder()didn't, so e.g.
sequenceEncoder("R", mode="property", property.set="MSWHIM", max.length=1)$flattenedsilently returned Cysteine's true MSWHIM values, notArginine's.
Verified against
Peptides::mswhimScores()/crucianiProperties()/protFP()— Peptides' own independent public scoring functions, not just immApex/
calculateProperty()internals — confirming the canonical-order reading isthe objectively correct one, not just a different convention.
Fix
Centralizes the canonical-AA reorder inside
.aa.property.matrix()itself(both the
.builtin_scalesbranch and thePeptides::AAdatabranch), soevery caller gets canonical order by construction instead of relying on each
call site to remember to reorder. This fixes both bug sites in one change:
R/sequenceEncoder.R:132andR/sequenceDecoder.R:110(the latter has theidentical root cause but wasn't previously flagged).
calculateProperty()'s own existing reorder is left in place — nowredundant but harmless/idempotent.
Tests
.aa.property.matrix()returns canonical column order for every supportedproperty set (
test-calculateProperty.R).sequenceEncoder()property mode matchesPeptides' own scoring functionsfor single-residue probes on all 3 previously-broken sets
(
test-sequenceEncoder.R).sequenceDecoder()property mode correctly identifies an amino acid from acube built directly from
Peptidesground truth — not viasequenceEncoder(), since a pure round trip through immApex's own(formerly buggy) functions would self-cancel this exact bug
(
test-sequenceDecoder.R).Version
1.7.1→1.7.2,NEWS.mdupdated.