Skip to content

[FIX] ASR: use a real PSD square root in asr_calibrate#114

Open
sappelhoff wants to merge 1 commit into
nbara:masterfrom
sappelhoff:fix/asr-calibrate-psd-sqrt
Open

[FIX] ASR: use a real PSD square root in asr_calibrate#114
sappelhoff wants to merge 1 commit into
nbara:masterfrom
sappelhoff:fix/asr-calibrate-psd-sqrt

Conversation

@sappelhoff

Copy link
Copy Markdown
Collaborator

asr_calibrate computed M = linalg.sqrtm(np.real(Uavg)), which only strips a pre-existing imaginary part but does not make Uavg positive-semidefinite. If the robust average covariance Uavg has even a tiny negative eigenvalue (realistic for average-referenced / rank-deficient EEG covariance means), scipy.linalg.sqrtm of the resulting indefinite symmetric matrix returns a complex M, which then silently propagates through eigh(M) into V and the threshold matrix T. asr_process already guards against this by taking np.real() of its eigen-quantities, but asr_calibrate did not.

Add _psd_sqrtm(), a real symmetric PSD square root that symmetrizes its input and clamps numerically-negative eigenvalues to zero before taking the root, and use it in asr_calibrate. For a genuinely PSD Uavg (the normal case) this is the same principal square root sqrtm would return, so calibration is numerically unchanged.

Testing

Added a regression test showing _psd_sqrtm stays real on a slightly indefinite matrix where sqrtm returns complex. Full tests/test_asr.py passes; ruff clean.

@codecov

codecov Bot commented Jul 13, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 83.15%. Comparing base (107cd1b) to head (ded6cee).

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #114      +/-   ##
==========================================
+ Coverage   83.11%   83.15%   +0.03%     
==========================================
  Files          25       25              
  Lines        2825     2831       +6     
==========================================
+ Hits         2348     2354       +6     
  Misses        477      477              

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@sappelhoff

Copy link
Copy Markdown
Collaborator Author

Note: includes the nonlinear_eigenspace reproducibility fix from #118. test_asr_class[*-riemann] compares incremental-vs-bulk output with a loose tolerance and is flaky on CI because that path used an unseeded RNG; this can trip on any PR regardless of what it changes. #118 is the root fix — once it merges first, this commit drops out on rebase.

@sappelhoff sappelhoff mentioned this pull request Jul 14, 2026
asr_calibrate computed M = linalg.sqrtm(np.real(Uavg)), which only strips a
pre-existing imaginary part but does not make Uavg positive-semidefinite. If
the robust average covariance Uavg has even a tiny negative eigenvalue
(realistic for average-referenced / rank-deficient EEG covariance means),
scipy.linalg.sqrtm of the resulting indefinite symmetric matrix returns a
complex M, which then silently propagates through eigh(M) into V and the
threshold matrix T.

asr_process already guards against this by taking np.real() of its
eigen-quantities after eigh, but asr_calibrate lacked the equivalent guard.
Add _psd_sqrtm(), a real symmetric PSD square root that symmetrizes its input
and clamps numerically-negative eigenvalues to zero before taking the square
root, and use it in asr_calibrate. For a genuinely PSD Uavg (the normal case)
this is the same principal square root sqrtm would return, so calibration is
numerically unchanged; when Uavg is slightly indefinite it now yields a real
M, and hence real V/T.
@sappelhoff
sappelhoff force-pushed the fix/asr-calibrate-psd-sqrt branch from 5b08383 to ded6cee Compare July 19, 2026 09:11
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