Skip to content

Stabilize singular empirical covariance precision - #8606

Open
sylvesterkaczmarek wants to merge 4 commits into
NVIDIA:mainfrom
sylvesterkaczmarek:bug-empirical-covariance-pinv
Open

Stabilize singular empirical covariance precision#8606
sylvesterkaczmarek wants to merge 4 commits into
NVIDIA:mainfrom
sylvesterkaczmarek:bug-empirical-covariance-pinv

Conversation

@sylvesterkaczmarek

@sylvesterkaczmarek sylvesterkaczmarek commented Sep 10, 2026

Copy link
Copy Markdown

Closes #8566

Summary

  • use a dtype-aware cutoff when computing the covariance pseudoinverse
  • apply the same cutoff to stored and on-demand precision matrices
  • add regression coverage for singular float32 covariance and Mahalanobis distances

Validation

  • python3 -m py_compile python/cuml/cuml/covariance/empirical_covariance.py python/cuml/tests/test_empirical_covariance.py
  • git diff --check

Signed-off-by: Sylvester Kaczmarek <16242628+sylvesterkaczmarek@users.noreply.github.com>
@copy-pr-bot

copy-pr-bot Bot commented Sep 10, 2026

Copy link
Copy Markdown

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

@github-actions github-actions Bot added the Cython / Python Cython or Python issue label Sep 10, 2026
@coderabbitai

coderabbitai Bot commented Sep 10, 2026

Copy link
Copy Markdown

Review Change StackReview Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 3b9508b4-cebd-40db-add2-13cbe8a729ec

📥 Commits

Reviewing files that changed from the base of the PR and between b84de5c and 2611612.

📒 Files selected for processing (1)
  • python/cuml/tests/test_empirical_covariance.py
🚧 Files skipped from review as they are similar to previous changes (1)
  • python/cuml/tests/test_empirical_covariance.py

Included review availability: Your plan provides up to 12 included reviews per hour; 10 remain after this review.


📝 Summary

Summary by CodeRabbit

  • Bug Fixes

    • Improved precision calculations for covariance matrices across supported data types, including rank-deficient and singular matrices.
    • Improved consistency of Mahalanobis distance results for float32 covariance data.
    • Improved numerical reliability when calculating precision matrices from singular covariance inputs.
  • Tests

    • Added regression coverage comparing precision and Mahalanobis distance results with scikit-learn for singular covariance matrices in multiple precision-storage configurations.

Walkthrough

Changes

Empirical covariance stability

Layer / File(s) Summary
Dtype-aware pseudoinverse integration
python/cuml/cuml/covariance/empirical_covariance.py
Adds _pinv with a dtype-specific tolerance. fit and get_precision use it to compute precision matrices.
Singular covariance regression coverage
python/cuml/tests/test_empirical_covariance.py
Adds parametrized comparisons with scikit-learn for singular float32 covariance input and both precision-storage configurations.

Estimated code review effort: 2 (Simple) | ~10 minutes

Severity of issue fixed: Medium

Merge Risk: ⚪ Minimal · up to b9e98

This change stabilizes singular covariance precision and Mahalanobis distances with dtype-aware behavior and regression coverage. No actionable merge-blocking risk is identified.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 66.67% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 9 functions across 2 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and concisely describes the primary change: stabilizing precision computation for singular empirical covariance matrices.
Description check ✅ Passed The description directly explains the dtype-aware pseudoinverse cutoff, its application to precision matrices, and the regression tests for singular float32 covariance.
Linked Issues check ✅ Passed The changes address issue #8566 by using a dtype-aware pseudoinverse cutoff for stable precision matrices and by adding regression coverage for Mahalanobis distances on singular covariance data.
Out of Scope Changes check ✅ Passed The implementation and test changes are limited to the singular covariance precision fix, related Mahalanobis regression coverage, and attribution updates. No unrelated code changes are evident.
✨ Finishing Touches 💡 1
🛠️ Fix failing CI checks 💡
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Comment @coderabbitai help to get the list of available commands.

Signed-off-by: Simon Adorf <sadorf@nvidia.com>

@csadorf csadorf left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. I ran all 44 EmpiricalCovariance tests and found no meaningful performance regression. I pushed the required pre-commit fix to this PR. Please run the pre-commit hooks for future commits.

return cp.dot(X_centered.T, X_centered) / X.shape[0]


def _pinv(covariance):

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I’d suggest investigating a Hermitian-specific pseudoinverse based on cp.linalg.eigh in a follow-up. That would also match scikit-learn’s use of scipy.linalg.pinvh. I ran some quick tests, and performance looked promising. This fix looks good, so we can merge as-is.

@csadorf csadorf added bug Something isn't working non-breaking Non-breaking change labels Sep 10, 2026
@csadorf

This comment has been minimized.

@csadorf

csadorf commented Sep 10, 2026

Copy link
Copy Markdown
Contributor

CI is only blocked by the known libcuml wheel repair failure tracked in #8603. All four wheel jobs fail because auditwheel cannot locate libkvikio.so; the same failures occur on main. All relevant build, test, style, and documentation checks passed.

sylvesterkaczmarek and others added 2 commits September 10, 2026 17:57
Signed-off-by: Sylvester Kaczmarek <16242628+sylvesterkaczmarek@users.noreply.github.com>
@csadorf

csadorf commented Sep 11, 2026

Copy link
Copy Markdown
Contributor

/ok to test b9e981f

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

Labels

bug Something isn't working Cython / Python Cython or Python issue non-breaking Non-breaking change

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[BUG] EmpiricalCovariance produces an unstable precision matrix and incorrect Mahalanobis distances for singular covariance

3 participants