You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Every NIPALS convergence test in the package except TPLS compares an absolute score-vector norm against a fixed tolerance. Whether the loop converges therefore depends on the magnitude of the data rather than on whether the scores have actually stopped moving. The correct, scale-invariant criterion (||t_new - t_old|| / ||t_new||) already exists in this repo, in _tpls._has_converged, and is simply not used by PCA / PLS / MBPCA / MBPLS.
Where
Line numbers as of ac52aab (branch claude/repo-error-audit-si62bq, PR #500).
src/process_improve/multivariate/_nipals.py, terminate_check - the shared PCA/PLS criterion
src/process_improve/multivariate/_mbpca.py:308 - tol = float(np.finfo(float).eps ** (9 / 10)) (~8.2e-15)
src/process_improve/multivariate/_mbpls.py:377 - tol = float(np.finfo(float).eps ** (6 / 7)) (~3.8e-14)
src/process_improve/multivariate/_tpls.py, _has_converged - the correct relative implementation to copy
Default md_tol is epsqrt (~1.49e-8), from _common.py
Evidence
defterminate_check(t_a_guess: np.ndarray, t_a: np.ndarray, iterations: int, settings: dict) ->bool:
"""Terminate the PCA iterative algorithm when any one of these conditions is True. #. scores converge: the norm between two successive iterations #. maximum number of iterations is reached """score_tol=np.linalg.norm(t_a_guess-t_a, ord=None)
converged=score_tol<settings["md_tol"] # <-- ABSOLUTE
Why it is wrong
The standard NIPALS criterion is relative. As written, the outcome depends on the units of the data:
Large-magnitude X (unscaled process data; scores of order 1e3): an absolute gap of 1.49e-8 may never be reachable in double precision, so the loop runs the full md_max_iter = 1000 iterations for every component - roughly 1000x slower than needed. Until PR Repo-wide error audit: fix statistical and robustness bugs #500 the PCA path did not even warn about this.
Tiny-magnitude X: the criterion is met on the first iteration regardless of whether anything converged, so a component can be returned essentially unconverged.
MBPCA / MBPLS make it worse by defaulting to a tolerance at machine precision on an absolute vector norm. For a score vector of norm ~10 (N=100 rows) the last-bit oscillation floor is around 1e-14, i.e. at or above the tolerance, so spurious "MBPCA NIPALS did not converge within max_iter=500" warnings are expected on ordinary, well-behaved data.
Users are told to scale with MCUVScaler before fitting, which hides the problem in the common path - but scale=False and the multiblock estimators are documented, supported entry points.
Suggested fix
Switch to the relative form already used by _tpls._has_converged:
||t_new - t_old|| / max(||t_new||, floor) < tol
with a small floor (see _nz in _common.py) so a fully-deflated all-zero score vector cannot divide by zero. Apply to terminate_check (which serves PCA and PLS) and to the MBPCA / MBPLS loops, and revisit the two machine-precision default tolerances once the criterion is relative - a relative tolerance near eps is unreachable in practice; epsqrt is the sane default.
Note terminate_check is shared, so a single change fixes both single-block estimators - check every caller (grep terminate_check src/) before changing the signature.
Acceptance criteria
Convergence decisions are invariant to a global rescale of the input: fitting X and 1000 * X takes the same number of iterations (assert on fitting_info_["iterations"]).
MBPCA / MBPLS no longer emit non-convergence warnings on ordinary well-conditioned data.
Existing fitted values are unchanged to tolerance on the scaled datasets already in the test suite (this is a stopping-rule change, not a maths change).
Tolerance defaults reviewed and documented.
Scope / non-goals
The max-iteration warnings themselves were fixed in PR #500 (the PLS condition could never fire; PCA had none) - do not duplicate that work. Do not change the NIPALS update equations.
References
Found during the repo-wide audit in PR #500, deferred because it changes iteration behaviour across four estimators and deserves its own benchmark/verification pass.
Summary
Every NIPALS convergence test in the package except TPLS compares an absolute score-vector norm against a fixed tolerance. Whether the loop converges therefore depends on the magnitude of the data rather than on whether the scores have actually stopped moving. The correct, scale-invariant criterion (
||t_new - t_old|| / ||t_new||) already exists in this repo, in_tpls._has_converged, and is simply not used by PCA / PLS / MBPCA / MBPLS.Where
Line numbers as of
ac52aab(branchclaude/repo-error-audit-si62bq, PR #500).src/process_improve/multivariate/_nipals.py,terminate_check- the shared PCA/PLS criterionsrc/process_improve/multivariate/_mbpca.py:308-tol = float(np.finfo(float).eps ** (9 / 10))(~8.2e-15)src/process_improve/multivariate/_mbpls.py:377-tol = float(np.finfo(float).eps ** (6 / 7))(~3.8e-14)src/process_improve/multivariate/_tpls.py,_has_converged- the correct relative implementation to copymd_tolisepsqrt(~1.49e-8), from_common.pyEvidence
Why it is wrong
The standard NIPALS criterion is relative. As written, the outcome depends on the units of the data:
md_max_iter = 1000iterations for every component - roughly 1000x slower than needed. Until PR Repo-wide error audit: fix statistical and robustness bugs #500 the PCA path did not even warn about this."MBPCA NIPALS did not converge within max_iter=500"warnings are expected on ordinary, well-behaved data.Users are told to scale with
MCUVScalerbefore fitting, which hides the problem in the common path - butscale=Falseand the multiblock estimators are documented, supported entry points.Suggested fix
Switch to the relative form already used by
_tpls._has_converged:with a small floor (see
_nzin_common.py) so a fully-deflated all-zero score vector cannot divide by zero. Apply toterminate_check(which serves PCA and PLS) and to the MBPCA / MBPLS loops, and revisit the two machine-precision default tolerances once the criterion is relative - a relative tolerance nearepsis unreachable in practice;epsqrtis the sane default.Note
terminate_checkis shared, so a single change fixes both single-block estimators - check every caller (grep terminate_check src/) before changing the signature.Acceptance criteria
Xand1000 * Xtakes the same number of iterations (assert onfitting_info_["iterations"]).Scope / non-goals
The max-iteration warnings themselves were fixed in PR #500 (the PLS condition could never fire; PCA had none) - do not duplicate that work. Do not change the NIPALS update equations.
References
Found during the repo-wide audit in PR #500, deferred because it changes iteration behaviour across four estimators and deserves its own benchmark/verification pass.