FEA Make LedoitWolf estimator array API compatible#33573
Conversation
The tests were written as part of a TDD approach during development, they cover things that the common tests also cover. So removing them to reduce the amount of duplication.
In some cases it returns flaot64 for float32 input. There are users who rely on this behaviour.
|
Hey @betatim, Sorry for dropping into the PR, but what you are working on is something super nice! At Pyriemann, with @agramfort and @qbarthelemy, we are using many of the covariance matrix implementations from scikit-learn and are also moving towards array API compatibility. I just would like to offer some assistance with the covariance part api compatibility, if necessary. |
|
I tried to find a macro issue to offer help, but I couldn't find a place that seemed appropriate. Please feel free to delete my comment, given that it's off-topic for your PR. |
No worries and welcome! It is fantastic to hear from someone using scikit-learn that says "this looks useful". I don't think there is a macro/mega issue related to this area of scikit-learn + array API support. I picked it more or less at random/as an exercise to see how much infrastructure would be missing. I think if you are interested in contributing to converting estimators/tools from https://scikit-learn.org/stable/api/sklearn.covariance.html go for it! It could be that the first step towards that is helping review this PR or picking another estimator from the list and converting it. I'd have to think about it, it probably depends on how much interdependency there is. Another thing that could be worth doing is creating an example for the gallery that shows off the array API support in I will create a mega-issue for |
bruAristimunha
left a comment
There was a problem hiding this comment.
if you do this, should unlock too the empiral covariance
Co-authored-by: Bru <b.aristimunha@gmail.com>
Convert _oas() and OAS.fit() to use array API compatible operations, following the same pattern as LedoitWolf from PR scikit-learn#33573.
Convert _oas() and OAS.fit() to use array API compatible operations, following the same pattern as LedoitWolf from PR scikit-learn#33573.
Convert _oas() and OAS.fit() to use array API compatible operations, following the same pattern as LedoitWolf from PR scikit-learn#33573.
If we flag it as supporting array API all the classes that inherit from it also get marked as supporting array API. This doesn't work, so we need to do this at the end when all estimators support array API.
|
I undid the changes to |
|
I can create a separate PR for this, if you want, @betatim. I fixed in my draft PR, small tricks are necessary for empirical covariance. |
Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org>
|
I've addressed all the comments, let me know what y'all think. Maybe we could get this wrapped up in time for the release? |
|
I ran the tests of this PR with DPNP on an Intel GPU with float32-only support and there are two failures barely above the rtol threshold for float64 inputs: https://github.com/probabl-ai/scikit-learn-intel-workflow/actions/runs/26148891699/job/76911036653 I think we should bump it up a bit (e.g. 1e-6 instead of 1e-7). |
|
@cakedev0 Actually there is something I don't understand in the above. Does your float32-only GPU: How is the case the following does not fail if the GPU is float32 only Also: I would have expected |
|
Not sure to understand exactly your message but, on my laptop (float32-only GPU): >>> dpnp.asarray(np.ones(3, dtype=np.float64), device="gpu")
array([1., 1., 1.], dtype=float32)And this test should definitely have been skipped, let's not bump the rtol but make sure it's skipped instead (I'm looking into that) |
cakedev0
left a comment
There was a problem hiding this comment.
Let's update the usage of _array_api_for_tests to skip tests for which the device doesn't support the dtype.
Co-authored-by: Arthur Lacote <arthur.lcte@gmail.com>
|
New Intel GPU workflow: https://github.com/probabl-ai/scikit-learn-intel-workflow/actions/runs/26577559267 EDIT: It's green! Let me fix the linter. |
|
@betatim this PR should be ready to merge once the following is addressed: #33573 (review) |
|
I fixed the use of |
cakedev0
left a comment
There was a problem hiding this comment.
This should discard the "request changes" review. (I'll avoid that next time)
|
Yeah, my impression is that the "request changes" feature isn't super useful for a project like scikit-learn. It ends up making it more complicated to merge things and I don't think I've ever experienced it where someone's feedback was ignored/not worked on because they "only" left it as a comment/normal review feedback. The social fabric is strong here :D |
|
@OmarManzoor and @ogrisel you both left 👍 , do you want to take a last look and then press merge? Or should we attract some more reviews? |
|
Merged! Thanks all! |
Reference Issues/PRs
Came up as part of pondering #33564
Towards #33584
What does this implement/fix? Explain your changes.
This makes
LedoitWolfarray API compatible. After this it might be easier to convert other covariance estimators as it also updates some of the basic infra shared by themTodo
ledoit_wolf(the function) in this PR as wellAI usage disclosure
I used AI assistance for:
Any other comments?