Skip to content

FIX Covariance matrix in BayesianRidge #31094

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged

Conversation

antoinebaker
Copy link
Contributor

@antoinebaker antoinebaker commented Mar 27, 2025

Fixes #31093

What does this implement/fix? Explain your changes.

Using the reduced SVD leads to an incorrect covariance matrix when n_samples < n_features.

This PR uses the full SVD to compute the posterior covariance matrix when n_samples < n_features, and keeps the reduced SVD when n_samples > n_features for performance in the large n_samples case.

The non-regression test test_bayesian_covariance_matrix is added.

@antoinebaker antoinebaker marked this pull request as draft March 27, 2025 16:14
Copy link

github-actions bot commented Mar 27, 2025

✔️ Linting Passed

All linting checks passed. Your pull request is in excellent shape! ☀️

Generated for commit: cdf9222. Link to the linter CI: here

@antoinebaker antoinebaker marked this pull request as ready for review March 28, 2025 09:04
@antoinebaker
Copy link
Contributor Author

The failing tests on the Linux debian_32bit seem unrelated to this PR, so I think it is ready for review.

FAILED tests/test_common.py::test_estimators[LinearRegression(positive=True)-check_sample_weight_equivalence_on_dense_data] - AssertionError: 
FAILED utils/tests/test_estimator_checks.py::test_check_estimator_clones - AssertionError:

@ogrisel
Copy link
Member

ogrisel commented Mar 28, 2025

I opened #31098 for the CI failure.

Copy link
Member

@ogrisel ogrisel left a comment

Choose a reason for hiding this comment

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

LGTM.

Copy link
Member

@jeremiedbb jeremiedbb left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks !

@jeremiedbb jeremiedbb enabled auto-merge (squash) April 13, 2025 13:50
@jeremiedbb jeremiedbb merged commit b90e09d into scikit-learn:main Apr 13, 2025
30 of 33 checks passed
lucyleeow pushed a commit to EmilyXinyi/scikit-learn that referenced this pull request Apr 23, 2025
Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org>
Co-authored-by: Jérémie du Boisberranger <jeremie@probabl.ai>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

The covariance matrix is incorrect in BayesianRidge
3 participants