Audit: test validity - #23
Merged
Merged
Conversation
orthogonal_procrustes returns an unconstrained orthogonal matrix (reflections allowed), so the per-component sign step never fired (0 flips across 2000+ bootstraps up to 200x150 columns). Output is bit-for-bit identical; the Procrustes alignment alone resolves sign.
Plant a known rank-1 structure and a near-degenerate two-component structure and assert the engine recovers them: top-LV direction, spectrum dominance, permutation significance in both directions, and bootstrap-ratio reliability incl. the Procrustes alignment. Replaces the tautological test_sign_consistency and fixes the vacuous 0.001 threshold in test_random_data_not_significant. Mutation-checked: n-1->n divisor, no-op permutation, and Q=eye each break an assertion.
SVD reconstruction (U diag(s) Vt == xcorr), orthonormality of the singular vectors, and bootstrap SE shrinking as injected SNR rises. Mutation-checked: scaling u in _decompose breaks reconstruction and orthonormality.
A PLS component's global sign is arbitrary and np.linalg.svd can return a different one across BLAS builds, flipping loadings, scores, and bootstrap ratios machine-to-machine. Flip each component so its largest-magnitude X loading is positive, making all outputs reproducible across machines. Guarded by a new test; not a semantic change (sign carries no scientific meaning).
Lock the result-defining numeric outputs of both run_pipeline methods and cross_validate_pipeline on the committed synthetic data. Tiered tolerances: tight for deterministic outputs (singular values, loadings, scores), absolute for permutation p-values, looser for bootstrap ratios. Regenerate deliberately by deleting tests/data/regression/. Mutation-checked: the n-1->n divisor breaks the snapshot.
Wrap the sklearn imports in cross_validate.py so a missing optional dependency raises a helpful 'install plsdo[cv]' message instead of a raw ImportError. import plsdo stays sklearn-free. Guarded by a test that simulates sklearn absence.
filter_lvs was hardcoded to 1.96 while --bsr-threshold only affected which loadings were plotted, so the flag silently failed to change which LVs survived. Pass the flag through so one knob means one thing; default 1.96 keeps existing behaviour unchanged. Update the help and docstring accordingly.
- _write_log now records numpy/scipy/scikit-learn versions for reproducibility (sklearn reported as 'not installed' when absent). - filter_lvs reuses self.significant_lvs instead of recomputing p_values < 0.05 (behaviour identical). - pin the tick locator before relabelling in plot_scores_scatter so runs are free of the matplotlib set_ticklabels warning.
- delete test_singular_values_descending (a property of numpy's svd) - delete the duplicate verbose-limit guard test (default-limit test already covers the firing path) - reduce the three subcommand-alias tests to fast stubbed-dispatch checks of method/func instead of full pipeline runs Kept the argparse interface and io/align guards: cheap, deliberate interface checks not worth the coverage risk to remove.
seaborn 0.13.2 (the latest release) calls bxp(vert=...), which matplotlib >= 3.10 deprecates; the fix is only on seaborn's unreleased main branch, so there is no version to bump to. Ignore this specific third-party PendingDeprecationWarning until a seaborn release lands. Test runs are now warning-clean.
- single X feature (n_x=1) runs end-to-end with one component - many-group discriminatory design gives min(n_groups, n_y) components - CV with fewer subjects than folds raises a clear ValueError
Comment the flip at both decision points and add a test asserting cross_validate_pipeline passes the continuous Y-matrix as run_cv's predictor and the group codes as the target (opposite to the discriminatory run_pipeline convention).
An additive K-factor dummy design is rank-deficient by K-1. Assert the degenerate trailing LV has ~zero singular value, is non-significant, is dropped by filter_lvs, and has ~zero loadings — verifying the design is harmless without resorting to contrast coding.
The bxp vert= fix is merged on seaborn main (PR #3820) but unreleased; seaborn has not shipped a release since 0.13.2 (Jan 2024), so note there is no firm timeline rather than implying an imminent bump.
… LVs Non-significant latent variables are discarded by filter_lvs and their permutation p-values and bootstrap ratios are not reproducible across BLAS builds: a near-zero or weak singular value puts the observed statistic in a dense pile of near-equal null values (flipping permutation counts), and a near-zero loading makes the ratio noise/noise. This drifted up to 0.27 on the Linux CI runners while passing on macOS. Restrict these two snapshots to the significant LVs; the deterministic snapshots (singular values, loadings, scores) still cover all LVs and catch the result-defining drift.
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.
No description provided.