Repository navigation
Add Hypothesis property-based tests, fix 3 bugs found - #555
Merged
Merged
Conversation
Adds ddmc/tests/test_properties.py with property-based tests (via Hypothesis) covering the core sequence-distance and preprocessing helpers in ddmc.pam250, ddmc.binomial, ddmc.motifs, and ddmc.logistic_regression. These tests found three real bugs, kept as strict xfail so CI stays green while documenting them (and will loudly flag a regression if the underlying bug is fixed without updating the test): - get_pam250_scores accumulates pairwise scores in an int8 array, which silently overflows for highly self-similar peptides (e.g. poly-W or poly-C), producing wrong (sometimes negative) similarity scores. - compute_control_pssm crashes with ValueError on a lowercase phosphoacceptor character, which is exactly what its real caller chain produces: BackgroundSeqs lowercases the phosphoacceptor, and DDMC.get_pssms(PsP_background=True) passes that output straight in. - normalize_cluster_centers centers along the wrong axis: the docstring says it zero-means each cluster across samples, but the double-transpose around StandardScaler actually zero-means each sample across clusters instead. The remaining property tests pass and pin down invariants (PWM symmetry/equivalence to the Biopython reference, one-hot encoding round-trips, phosphosite-type counting) that should keep holding as this code changes. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
- get_pam250_scores: widen the pairwise-score accumulator from int8 to int32 so it no longer silently overflows (and wraps to a negative score) for highly self-similar peptides such as poly-tryptophan or poly-cysteine motifs. - compute_control_pssm: upper-case each residue before the AAlist lookup, so it no longer crashes on the lowercased phosphoacceptor that BackgroundSeqs (its actual caller, via DDMC.get_pssms(PsP_background=True)) always produces. - normalize_cluster_centers: drop the erroneous transpose-around- StandardScaler, which was centering each sample across its clusters instead of centering each cluster across samples as documented. Turns the corresponding xfail(strict=True) tests in ddmc/tests/test_properties.py into ordinary passing regression tests. Co-Authored-By: Claude Sonnet 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.
Summary
Adds
ddmc/tests/test_properties.py: property-based tests (via Hypothesis) for the core sequence-distance and preprocessing helpers inddmc.pam250,ddmc.binomial,ddmc.motifs, andddmc.logistic_regression. Instead of hand-picked examples, each test generates many random-but-valid inputs and checks an invariant that should hold for all of them (symmetry, round-tripping, matching a reference implementation, etc.).Adds
hypothesisto thedevdependency group.Running these tests turned up three real bugs, all now fixed in this PR:
get_pam250_scoressilently overflowedint8. The pairwise PAM250 similarity matrix was accumulated into anint8array. A length-11 peptide's self-similarity score can exceedint8's 127-value ceiling for highly self-similar residues — e.g. 11 tryptophans score11 * 17 = 187, which wrapped around to-69. This meant highly conserved/repetitive motifs (poly-W, poly-C) could be scored as less similar to themselves than to unrelated sequences, silently corrupting the PAM250 distance metric used byDDMC(distance_method="PAM250").Fix: widen the accumulator from
int8toint32(ddmc/pam250.py).compute_control_pssmcrashed on the exact input its real caller produces. It looked up each residue withAAlist.index(aa), which raisesValueErrorfor a lowercase letter. Butddmc.binomial.BackgroundSeqs— the only placecompute_control_pssmis actually called from, viaDDMC.get_pssms(PsP_background=True)(ddmc/clustering.py:281-282) — always lowercases the phosphoacceptor of the sequences it returns. SoDDMC.get_pssms(PsP_background=True)was broken end-to-end, raisingValueError: 'y' is not in list(or's'/'t').Fix: upper-case each residue before the
AAlistlookup (ddmc/motifs.py).normalize_cluster_centerscentered along the wrong axis. Its docstring says it zero-means each cluster (column) across samples (rows) — the usual per-feature standardization before classification. But the implementation wrappedStandardScaler(with_std=False).fit_transformincenters.T ... .T, which instead zero-meaned each sample across that sample's clusters, leaving the per-cluster mean across samples unchanged (and generally nonzero).Fix: call
StandardScaler(with_std=False).fit_transform(centers)directly, without the transposes (ddmc/logistic_regression.py).The other seven property tests passed from the start and pin down invariants worth protecting going forward —
fast_position_weight_matrixnumerically matching the Biopython reference PWM,GenerateBinarySeqID's one-hot encoding round-tripping to the original sequence,CountPsiteTypesaccounting for every input sequence, and PAM250-score symmetry. All ten tests, including the three that exposed bugs, now pass as ordinary regression tests (noxfailmarkers needed).Test plan
uv run pytest ddmc/tests/test_properties.py -v— 10 passeduv run pytest ddmc/tests/ -q --deselect ddmc/tests/test_datasets.py— 54 passed (no regressions;test_datasets.pydeselected as it needs network-fetched data not available in this environment)uv run ruff check ddmc/— clean🤖 Generated with Claude Code