ENH allow shrunk_covariance to handle multiple matrices at once - #25275
Conversation
|
Thx @agramfort if you can help the process! |
agramfort
left a comment
There was a problem hiding this comment.
@qbarthelemy can you add a changelog entry?
here is a just for a bug
can you see why CIs are red?
thx @qbarthelemy
|
Thx for your review @agramfort ! Everything seems OK. |
|
Can you see why CIs fail? |
|
Errors do not seem to be related to my modifications.
|
glemaitre
left a comment
There was a problem hiding this comment.
Instead of adding this validation, we should instead use the parameter validation framework: #24862
Before to be able to do that, we will need a small refactoring to have the function calling the class as we did for other estimator/function. I will fix that in another PR and we can build upon it here.
| ---------- | ||
| emp_cov : array-like of shape (n_features, n_features) | ||
| Covariance matrix to be shrunk. | ||
| emp_cov : array-like of shape (..., n_features, n_features) |
There was a problem hiding this comment.
| emp_cov : array-like of shape (..., n_features, n_features) | |
| emp_cov : array-like of shape (n_features, n_features) or \ | |
| (n_matrices, n_features, n_features) |
There was a problem hiding this comment.
Suggestions about docstring seem to restrict usage to 2D and 3D arrays, whereas function is now able to process any nd array.
There was a problem hiding this comment.
What is the use case of n-D shrinkage? Since the covariance matrix is always a 2-D matrix, I don't really see when you will pass a 4-D array, for instance.
There was a problem hiding this comment.
Most of NumPy and SciPy functions use (..., n, n) to indicate that they can process ndarrays,
like numpy.linalg.eig and scipy.linalg.expm for example.
Use cases belong to users. But, for an example with a 4D array, shape (k, m, n, n), one might want to shrunk k sets, each set containing of m covariance matrices. I have tested, and code is ok.
For me, description should not restrain actual usage. But, as you wish.
There was a problem hiding this comment.
I was thinking more of restraining on purpose the usage that we have in scikit-learn.
But we can go this road and see what other reviewers think.
| shrunk_cov.flat[:: n_features + 1] += shrinkage * mu | ||
| mu = np.trace(emp_cov, axis1=-2, axis2=-1) / n_features | ||
| mu = np.expand_dims(mu, axis=tuple(range(mu.ndim, emp_cov.ndim))) | ||
| shrunk_cov += shrinkage * mu * np.eye(n_features) |
There was a problem hiding this comment.
Here, we materialize the eye matrix. If we restrain to a 3-D matrix then, we can efficiently do the in place addition with flattening as previously done.
| ......................... | ||
|
|
||
| - |Enhancement| Allow :func:`covariance.shrunk_covariance` to process | ||
| multiple covariance matrices at once. |
There was a problem hiding this comment.
Then we need to mention somehow that we handle nd-array.
| ---------- | ||
| emp_cov : array-like of shape (n_features, n_features) | ||
| Covariance matrix to be shrunk. | ||
| emp_cov : array-like of shape (..., n_features, n_features) |
There was a problem hiding this comment.
I was thinking more of restraining on purpose the usage that we have in scikit-learn.
But we can go this road and see what other reviewers think.
|
Tests fail, due to np.expand_dims. |
|
Indeed, it was introduced in NumPy 1.18 and we have minimal dependencies set to 1.17.3. So for scikit-learn 1.3, we will be able to get with the current implementation. We might need to delay the merge of the feature depending on the bugfix release, but I will put this feature in the milestone. |
|
Actually we're not bumping the numpy min version for 1.3. Let's target this for 1.4 and we'll merge this when we bump the min version. |
50e7b8f to
38443e7
Compare
Co-authored-by: Alexandre Gramfort <alexandre.gramfort@m4x.org>
Co-authored-by: Guillaume Lemaitre <g.lemaitre58@gmail.com>
Co-authored-by: Guillaume Lemaitre <g.lemaitre58@gmail.com>
c9ad783 to
de0ba82
Compare
|
I open #27910 to bump the version of NumPy. Once merged, this PR should be mergeable. |
|
Merging since everything is green. Thanks @qbarthelemy |
…it-learn#25275) Co-authored-by: Alexandre Gramfort <alexandre.gramfort@m4x.org> Co-authored-by: Guillaume Lemaitre <g.lemaitre58@gmail.com> Co-authored-by: Jérémie du Boisberranger <34657725+jeremiedbb@users.noreply.github.com>
Reference Issues/PRs
None.
What does this implement/fix? Explain your changes.
Current version of
shrunk_covarianceprocesses only one covariance matrix.This PR aims to handle ndarrays with n>=2.
Any other comments?
Example with a 3D array.
Comput time = 0.0105 +/- 0.0004
Comput time = 0.0032 +/- 0.0001