Skip to content

Use Array API in r2_score #27102

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

Closed
wants to merge 45 commits into from

Conversation

elindgren
Copy link
Contributor

Reference Issues/PRs

One of the items outlined in #26024.

What does this implement/fix? Explain your changes.

Migrates r2_score to use the Array API as outlined in #26024.

This PR also introduces the function _average that mimics the functionality of np.average for weighted averages, as that is not in the Array API spec. _average can be found under utils/_array_api.py.

Any other comments?

None

@elindgren elindgren changed the title update r2 score to use the array API, and write initial tests Update r2 score to use the array API Aug 18, 2023
@github-actions
Copy link

github-actions bot commented Aug 18, 2023

✔️ Linting Passed

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

Generated for commit: 1bf557d. Link to the linter CI: here

@elindgren elindgren changed the title Update r2 score to use the array API Use Array API in r2_score Aug 18, 2023
denominator = (
weight * (y_true - np.average(y_true, axis=0, weights=sample_weight)) ** 2
).sum(axis=0, dtype=np.float64)
numerator = xp.sum(weight * (y_true - y_pred) ** 2, axis=0, dtype=xp.float64)
Copy link
Member

Choose a reason for hiding this comment

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

Since the end goal is to compute a scalar and we want float64 precision but some namespace / device combinations might not support this (e.g. pytorch / MPS only support float32 operations), I think we need to move to CPU before computing the float64 sum.

See the following for a similar case:

Note that this will make the computation slightly less efficient for devices that do support float64 arithmetic, but maybe this is a pragmatic fix.

Alternatively, we could attempt to write a utility in _array_api.py that dynamically inspects if a given namespace / device combo can support float64 operation (and cache the result) so as to trigger the to device="cpu" copy only in cases when it's actually needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I implemented this, but I ran into issues with CuPy. Apparently, device="cpu" does not work, see the below error message. I made a hacky workaround for this that keeps data on the GPU only for CuPy, but I'm not happy with it. Any ideas?

Screenshot from 2023-09-08 17-17-14

The hacky workaround in question:

https://github.com/elindgren/scikit-learn/blob/a4dd5944c8c88f2b321ee31c3ca835dc39ca99c6/sklearn/metrics/_regression.py#L1024-L1026

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I cherry-picked @betatim's solution from #27232 to solve this issue. Now the result is automatically cast to the most accurate float that the platform supports.

@ogrisel
Copy link
Member

ogrisel commented Aug 19, 2023

We probably need a new entry in doc/whats_new/v1.4.rst to document the enhancement to r2_score.

@ogrisel
Copy link
Member

ogrisel commented Sep 7, 2023

@elindgren I merged #27137 and synced your branch with main. I think there are things that can be consolidated, with respect to testing and documenting the changes in the changelog for instance.

@elindgren
Copy link
Contributor Author

@elindgren I merged #27137 and synced your branch with main. I think there are things that can be consolidated, with respect to testing and documenting the changes in the changelog for instance.

Thanks! Sorry for the radio silence, picking this back up.

elindgren and others added 6 commits October 5, 2023 10:21
Co-authored-by: Tim Head <betatim@gmail.com>
Co-authored-by: Tim Head <betatim@gmail.com>
Some Array API compatible libraries do not have a device called 'cpu'.
Instead we try and detect the lib+device combination that does not
support float64.
@fcharras
Copy link
Contributor

fcharras commented Dec 5, 2023

I've opened a follow-up PR at #27904

(of course it keeps your commits and includes your name in the changelog)

It is hard to contribute when the guidelines and signaling are unclear.

Yet you've done well and your work has been very useful. I can also see now how awful of a maze it is when beginning to tackle dtype and device management. Can't say for sure the solutions I'm proposing in #27904 are more likeable.

fcharras and others added 25 commits December 5, 2023 16:29
…se fast code paths do not generalized to nd inputs
@ogrisel
Copy link
Member

ogrisel commented Jan 10, 2024

Closing this PR in favor of #27904 which includes the original commits of this PR and credits the work of @elindgren in the changelog. I think we are converging on a consensus.

@ogrisel ogrisel closed this Jan 10, 2024
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.

7 participants