Skip to content

fix: bootstrap ratios from raw saliences, not S-scaled loadings - #26

Closed
eilidhmacnicol wants to merge 2 commits into
mainfrom
fix/bsr-no-s-scaling
Closed

fix: bootstrap ratios from raw saliences, not S-scaled loadings#26
eilidhmacnicol wants to merge 2 commits into
mainfrom
fix/bsr-no-s-scaling

Conversation

@eilidhmacnicol

Copy link
Copy Markdown
Contributor

Summary

Bootstrap ratios were being formed from S-scaled loadings (U·S, S·Vᵀ) instead of the raw saliences (U, Vᵀ). The canonical PLSC bootstrap ratio (McIntosh & Lobaugh 2004; Krishnan et al. 2011; the pyls reference implementation) is salience / bootstrap SE, with singular values playing no part.

Because S was re-estimated in every resample, its sampling variability leaked into the bootstrap SE, deflating the ratios — most on weaker / non-dominant latent variables. The dominant LV is largely unaffected (its singular value is stable, so S nearly cancels); the error therefore caused under-reporting (false negatives) on secondary effects, never false positives.

Changes

  • plsdo/core.py
    • fit(): u_loadings/vt_loadings are now the raw saliences u/vt (no * s).
    • bootstrap(): the bootstrap distribution collects the Procrustes-aligned raw singular vectors, so u_se/vt_se are salience SEs and BSR = salience / SE. Removed the now-unused boot_s.
    • BSR = loadings / SE line and all of pipeline.py are unchanged: the meanings of u_loadings/u_se/u_bootstrap_ratios shift together and stay internally consistent (loading plots, error bars, filter_lvs).
    • Fixes both correlational and discriminatory pipelines (shared SVD engine).
  • tests/test_core.py
    • test_loadings_are_scaled_vectorstest_loadings_are_raw_saliences.
    • Dropped the u_loadings[:, -1] ≈ 0 assertion in the inert-LV test (only true under S-scaling; the LV's harmlessness is still guarded by the significance / filter_lvs assertions).
  • tests/data/regression/: regenerated the 8 affected loadings/BSR snapshots (both methods). Singular values, p-values, subject scores and CV snapshots re-matched on regeneration, confirming nothing else moved.

Verification

  • pytest tests/ → 208 passed.
  • ruff check / ruff format --check clean.

🤖 Generated with Claude Code

@eilidhmacnicol

Copy link
Copy Markdown
Contributor Author

Code review

Found 1 issue (blocker):

  1. Y-side bootstrap ratios collapse to ~5e11 (effectively infinite) whenever the Y block has no more features than the X block (n_y <= n_x) — the typical correlational case. Q is solved by orthogonal_procrustes(boot_vt.T, self.vt.T), and when n_components == n_y, boot_vt is a square orthogonal matrix, so the Procrustes solution maps it exactly onto the reference: Q.T @ boot_vt == self.vt for every resample. The bootstrap distribution of aligned_vt therefore has zero spread, vt_se ~ 1e-16 breaches the eps = 1e-12 floor, and vt_bootstrap_ratios = vt_loadings / eps ~ 5e11 for every feature. This silently bypasses the Y-side reliability gate in filter_lvs (y_reliable becomes vacuously true). The old S-scaling masked this because S does not commute with Q, so it kept genuine variance in the distribution; removing it exposes the latent flaw.

Reproduced on tests/data/brain.csv/behaviour.csv (n_x=5, n_y=4): vt_se all ~1e-16, vt_bootstrap_ratios all ~5e11, while u_se is healthy (~0.27). The regenerated snapshots committed in this PR baked in these degenerate values, so test_regression.py compares broken-against-broken under the LOOSE tolerance and passes — the guard does not catch it.

The mathematical motivation (raw-salience BSR, no S) is correct; the alignment step needs reworking so both sides retain genuine resampling variance (e.g. derive the rotation from a non-degenerate quantity rather than the square Vt block, following the pyls/McIntosh convention). Do not merge or release until the Y-side SE is non-degenerate.

plsdo/plsdo/core.py

Lines 146 to 151 in 16d1a0c

# Procrustes: rotate bootstrap Vt to align with reference
Q, _ = orthogonal_procrustes(boot_vt.T, self.vt.T)
aligned_u = boot_u @ Q
aligned_vt = Q.T @ boot_vt

🤖 Generated with Claude Code

- If this code review was useful, please react with 👍. Otherwise, react with 👎.

@eilidhmacnicol

Copy link
Copy Markdown
Contributor Author

This was spearheaded by an LLM hallucination. On further investigation, s-scaled loadings are preferred, so won't fix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant