fix: low-risk correctness audit batch (defensive copies, crash fixes, validation, config)#346
Merged
Merged
Conversation
…lidation gaps, messages) Part of #342. None of these change published numeric output. - Defensive copies (shared-object mutation): load_dataset(gap) and load_features no longer return/mutate the shared lru_cache frame; get_df_feat no longer rewrites the caller's df_parts in place. - Crash-on-edge-input: read_fasta raises a clear ValueError on pre-header text (was KeyError); SeqMut.mutate re-joins scored rows on (entry, mutation) so duplicate labels across entries no longer crash; CPP.eval clamps the cluster search so a single-feature set can't hit KMeans(0). - comp_seq_sim self-similarity diagonal is 100 (the [0,100] scale), not 1. - Plotting/display: marker_size length check is reachable (ValueError, not a later IndexError); display_df row/col selector is 0..n-1; weight_bold no longer renders thinner than normal; feature-map show_only_max uses imp_bar_label_type; profile y-padding uses max(0, ...); plot_legend validation errors report the real parameter names. - Validator/message hygiene: check_metric and check_match_X_n_clusters messages corrected; load_scales name=; comp_seq_sim paren; explicit categories on previously bare warnings.warn calls. Regression tests: tests/unit/test_correctness_batch_342.py (+ updated the check_match_X_n_clusters message assertion in test_aac_branch.py). Held (not in this commit): config.py options-validation group (verbose/ random_state/name_tmd/LOKY) — CONFIRM-FIRST surface, awaiting sign-off. Deferred: sliding-window over-large-window guard — needs a silent-empty- vs-raise decision (current silent-empty behavior is test-encoded). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Completes the config.options group of #342 (CONFIRM-FIRST, approved): - _check_option validates the incoming verbose/random_state candidate directly instead of routing through the runtime resolvers, which read the current global and skipped validating a new value once one was set. - name_tmd is now validated like name_jmd_n / name_jmd_c (any name_* -> str). - check_n_jobs undoes only the loky CPU cap it set itself when allow_multiprocessing is re-enabled, so it is not left stuck at 1 and a user's own LOKY_MAX_CPU_COUNT is never clobbered. Tests added to tests/unit/test_correctness_batch_342.py. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
- SeqMut.mutate: score in mutation order (build_scan_output/_delta_table gain a sort=False path) and assign positionally, so duplicate mutation rows no longer desync/crash the previous (entry, mutation) re-join. - config check_n_jobs: on allow_multiprocessing=False remember the user's prior LOKY_MAX_CPU_COUNT and restore it on re-enable (was overwritten with '1' then popped, losing the user's value). - Revert three rendered-output plot changes that don't belong in a no-output- change batch (feature_map importance-bar ticks, profile y-axis padding, weight_bold spine/tick widths); deferred to the output-changing decision set. comp_seq_sim's diagonal correction (1 -> 100 on the [0,100] scale) is kept as the sole intentional value fix (pro function, unanchored, no test pinned it). Tests: SeqMut duplicate-row regression + LOKY save/restore assertion. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
… disabled window Re-review follow-up: check_n_jobs only undoes its own cap on re-enable if the value is still '1'; if the user set their own LOKY_MAX_CPU_COUNT (e.g. for another loky/ joblib library) while multiprocessing was disabled, it is left untouched. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #346 +/- ##
==========================================
+ Coverage 94.83% 94.84% +0.01%
==========================================
Files 196 196
Lines 18767 18783 +16
Branches 3175 3181 +6
==========================================
+ Hits 17797 17815 +18
+ Misses 633 632 -1
+ Partials 337 336 -1
🚀 New features to boost your workflow:
|
…test comments Per PR review: the test comments no longer cite the issue number or internal audit finding IDs; each test's comment now plainly states what it pins. Co-Authored-By: Claude Opus 4.8 <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
1 participant
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.
Part of #342. Implements the low-risk, non-output-changing portion of the July 2026
correctness audit plus the
config.optionsgroup. No CPP regression-anchor regoldenrequired. (The output/behavior-changing findings are tracked separately — see Deferred.)
Included fixes
Defensive copies (shared-object mutation)
load_dataset(non_canonical_aa="gap")andload_featuresno longer return/mutate theshared
lru_cacheDataFrame;get_df_featno longer rewrites the caller'sdf_partsin place.
Crash-on-edge-input
read_fastaraises a clearValueErroron pre-header text (was a crypticKeyError).SeqMut.mutate(df_feat=…)scores in mutation order and aligns results positionally, soduplicate / label-colliding mutation rows no longer desync or crash.
CPP.evalclamps the cluster search so a single-feature set can't reachKMeans(0).Validation / messages / config
display_dfrow/col selector is0..n-1; themarker_sizelength check is reachable(
ValueError, not a laterIndexError);plot_legendreports the real parameter names;check_metric/check_match_X_n_clustersmessages corrected;load_scalesname=;comp_seq_simf-string paren; explicit categories on previously barewarnings.warn.config.options(CONFIRM-FIRST, approved): validate the incomingverbose/random_statecandidate (not the current global); validate
name_tmd;check_n_jobssaves and restoresthe user's
LOKY_MAX_CPU_COUNTinstead of leaving it stuck at1after re-enablingmultiprocessing.
Intentional value correction
comp_seq_simself-similarity diagonal is100(matching the[0, 100]scale of everyoff-diagonal cell), not
1. Pro function, no regression anchor, no test pinned the value.Deferred (output/behavior-changing — need a decision, not in this PR)
weight_boldspine/tick widths) — they change committed figures and need notebook re-execution.
These join the output-changing set tracked in #343 (CPP redundancy filter, TreeModel
per-round seeding, BH p-value monotonicity).
Tests
New
tests/unit/test_correctness_batch_342.pyplus a SeqMut duplicate-row regression intest_seqmut_mutate.py. Touched-area suites pass locally (data_handling, config, plotting,cpp_plot, aaclust, seq_analysis_pro, protein_engineering, sequence_feature) and the smoke gate.
Review
xhigh multi-agent
/code-review(its findings — the SeqMut re-join edge, the config LOKYhandling, and three output-changing plot changes — were fixed or reverted) and
/security-review(clean).🤖 Generated with Claude Code