ENH: Make roc_curve array API compatible #30878

lithomas1 · 2025-02-22T16:56:32Z

Reference Issues/PRs

xref #26024

What does this implement/fix? Explain your changes.

Makes roc_curve array API compatible.

Any other comments?

github-actions · 2025-02-22T16:57:51Z

✔️ Linting Passed

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

_{Generated for commit: dbdfbd5. Link to the linter CI: here}

…curve

OmarManzoor

Thanks for the PR @lithomas1

I added some initial comments.

sklearn/metrics/_ranking.py

sklearn/utils/_array_api.py

sklearn/utils/validation.py

lithomas1 · 2025-03-18T00:33:27Z

Thanks for the review, and sorry for the slow reply.

I addressed the device issues (a MPS run on my Intel MBP uncovered some more issues that I fixed).

OmarManzoor

Thanks for the updates @lithomas1.
Mostly this looks good. However let's consider waiting for the array-api-extra PR

sklearn/metrics/_ranking.py

sklearn/metrics/tests/test_common.py

sklearn/utils/_array_api.py

Co-authored-by: Omar Salman <omar.salman@arbisoft.com>

sklearn/externals/array_api_extra/_lib/_funcs.py

OmarManzoor

LGTM. Thanks @lithomas1

CC: @ogrisel @betatim for a second review

doc/whats_new/upcoming_changes/array-api/30878.feature.rst

sklearn/utils/extmath.py

Co-authored-by: Omar Salman <omar.salman@arbisoft.com>

lucyleeow · 2025-06-05T01:08:35Z

sklearn/utils/tests/test_extmath.py

+    arr_np = np.asarray(
+        [[1, 2e-9, 3e-9] * int(1e6)],
+    )
+    arr_xp = xp.asarray(
+        arr_np, dtype=getattr(xp, dtype) if dtype is not None else dtype
+    )


Why not do:

Suggested change

arr_np = np.asarray(

[[1, 2e-9, 3e-9] * int(1e6)],

)

arr_xp = xp.asarray(

arr_np, dtype=getattr(xp, dtype) if dtype is not None else dtype

)

arr_np = np.asarray(

[[1, 2e-9, 3e-9] * int(1e6)], dypte=dtype

)

arr_xp = xp.asarray(arr_np)

?

betatim · 2025-06-05T07:13:26Z

sklearn/utils/tests/test_extmath.py

+        assert_allclose(
+            _convert_to_numpy(stable_cumsum(arr_xp, axis=axis), xp),
+            np.cumsum(arr_np, axis=axis),
+        )


Brain wave just now: maybe instead of creating arr_np and using _convert_to_numpy we could use xpx.isclose? One reason to have array api extra is to slowly start using what it has to offer instead of having our own way. Though I think we have to call .all() on the result of xpx.isclose because it returns an array, not just one bool? @lucascolley

You could, but then you would have to implement all of the extra features of assert_allclose on top.

We are looking to expose the following as xpx.testing.assert_close when it has everything we need and the public API is agreed upon: https://github.com/data-apis/array-api-extra/blob/main/src/array_api_extra/_lib/_testing.py#L213-L276. It converts to NumPy and uses np.testing at the minute, but feasibly it could use things from other backends instead down the line.

It would be really useful if someone could bump xpx to v0.8.0 in sklearn and try using those private functions, to see what is missing.

Ah ok, I thought that xpx.isclose is that replacement for assert_allclose :-/

No, xpx.isclose covers https://numpy.org/doc/2.1/reference/generated/numpy.isclose.html, which is used outside of testing (of course it can be used in tests, but the assertions are more feature-complete)

It would be really useful if someone could bump xpx to v0.8.0 in sklearn and try using those private functions, to see what is missing.

x-ref data-apis/array-api-extra#17, please chime in there if anyone tries this out!

sklearn/utils/tests/test_extmath.py

lithomas1 · 2025-06-05T14:47:46Z

Ugh looks like something weird going on with precision for cupy.

Is it OK to just skip the test for cupy (this isn't ideal, but we have some confirmation the check works on GPU for torch already)?
Maybe there's also potentially a test we can steal from cuml?

OmarManzoor · 2025-06-05T14:50:02Z

Ugh looks like something weird going on with precision for cupy.

Is it OK to just skip the test for cupy (this isn't ideal, but we have some confirmation the check works on GPU for torch already)? Maybe there's also potentially a test we can steal from cuml?

We weren't testing for this warning before or were we?

lithomas1 · 2025-06-05T16:17:01Z

Just for numpy, since stable_cumsum wasn't array API compatible before.

OmarManzoor · 2025-06-05T17:36:43Z

Just for numpy, since stable_cumsum wasn't array API compatible before.

From my experimentation this warning isn't too common on cuda and it seemed hard to reproduce. Do you think we should maybe test it only for specific conditions and maybe not for cupy or torch?

Also I think some other opinions might be valuable. I'll also tag @ogrisel for his input.

OmarManzoor · 2025-06-11T10:56:06Z

@lithomas1 Let's do this for stable_cumsum. We can keep the original test that we had with just numpy that would also test the warning. For the array api test, let's just test for equivalence of the function with numpy outputs.

ogrisel · 2025-06-12T12:27:18Z

Also I think some other opinions might be valuable. I'll also tag @ogrisel for his input.

Thanks for the ping. I replied in the original thread above: #30878 (comment)

For the particular case of this PR, I think we should revert the changes under sklearn/utils and just call xp.cumsum directly in the code that computes the ROC curve.

OmarManzoor · 2025-06-12T13:26:54Z

For the particular case of this PR, I think we should revert the changes under sklearn/utils and just call xp.cumsum directly in the code that computes the ROC curve.

That sounds good. Thanks.

ogrisel · 2025-06-13T08:04:55Z

As shown by @lesteve in #31533 (comment), casting to xp.float64 before computing the cumsum can be required to get roc_auc_score to return the correct value on some dataset. This PR should add a non-regression test based on Loïc's comment if it's not already present in our test suite.

…cision with float32 weights

OmarManzoor · 2025-06-16T12:04:44Z

@ogrisel I made the updates that were discussed. Though I added a non regression test since it needs a large number of samples it takes a bit of time to run. Could you kindly have a look at the changes?

OmarManzoor · 2025-06-16T13:05:48Z

The failed test is due to the fact that the size of the array allocation is too large because of the large number of samples

OmarManzoor · 2025-06-17T05:45:11Z

@lesteve Do you have any suggestions on how we can keep this test?

ogrisel

Given the cost to maintain a proper non-regression test, I think an inline comment will do.

sklearn/metrics/_ranking.py

sklearn/metrics/tests/test_ranking.py

sklearn/metrics/tests/test_common.py

Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org>

OmarManzoor · 2025-06-18T15:01:54Z

I think since the suggestions have been resolved with respect to this PR, let's merge this. Thank you for the work @lithomas1

ENH: Make roc_curve array API compatible

4ba176c

github-actions bot added module:metrics module:utils labels Feb 22, 2025

lithomas1 added 2 commits March 1, 2025 19:28

add whatsnew

b7992cc

Merge branch 'main' of github.com:scikit-learn/scikit-learn into roc-…

8d2d989

…curve

lithomas1 force-pushed the roc-curve branch from 9206359 to 8d2d989 Compare March 2, 2025 00:29

lithomas1 added 2 commits March 1, 2025 20:54

try to fix for oldest numpy

54892c0

fix argsort for old numpy

e87c6f8

lithomas1 marked this pull request as ready for review March 2, 2025 03:48

OmarManzoor reviewed Mar 2, 2025

View reviewed changes

virchan added the Array API label Mar 5, 2025

lithomas1 added 2 commits March 17, 2025 19:26

update following code review

0980486

fix mps

07a910a

lithomas1 requested a review from OmarManzoor March 18, 2025 00:33

OmarManzoor reviewed Mar 22, 2025

View reviewed changes

sklearn/metrics/_ranking.py Outdated Show resolved Hide resolved

sklearn/metrics/_ranking.py Show resolved Hide resolved

sklearn/metrics/tests/test_common.py Outdated Show resolved Hide resolved

sklearn/utils/_array_api.py Outdated Show resolved Hide resolved

OmarManzoor added the CUDA CI label Mar 22, 2025

github-actions bot removed the CUDA CI label Mar 22, 2025

lithomas1 and others added 3 commits March 23, 2025 21:32

Apply suggestions from code review

619b54d

Co-authored-by: Omar Salman <omar.salman@arbisoft.com>

Merge branch 'main' into roc-curve

5322d94

fix tests

df7bf52

lithomas1 commented Mar 25, 2025

View reviewed changes

sklearn/externals/array_api_extra/_lib/_funcs.py Outdated Show resolved Hide resolved

Merge branch 'main' into roc-curve

0e3b7f9

lithomas1 requested review from OmarManzoor and lucyleeow March 26, 2025 19:27

OmarManzoor added the CUDA CI label Apr 6, 2025

github-actions bot removed the CUDA CI label Apr 6, 2025

OmarManzoor approved these changes Apr 6, 2025

View reviewed changes

doc/whats_new/upcoming_changes/array-api/30878.feature.rst Outdated Show resolved Hide resolved

sklearn/utils/extmath.py Outdated Show resolved Hide resolved

sklearn/utils/extmath.py Outdated Show resolved Hide resolved

Apply suggestions from code review

bdcd3ef

Co-authored-by: Omar Salman <omar.salman@arbisoft.com>

lucyleeow reviewed Jun 5, 2025

View reviewed changes

betatim reviewed Jun 5, 2025

View reviewed changes

OmarManzoor reviewed Jun 5, 2025

View reviewed changes

sklearn/utils/tests/test_extmath.py Outdated Show resolved Hide resolved

Update sklearn/utils/tests/test_extmath.py

92dc114

OmarManzoor added the CUDA CI label Jun 5, 2025

github-actions bot removed the CUDA CI label Jun 5, 2025

ogrisel mentioned this pull request Jun 12, 2025

RFC: stop using scikit-learn stable_cumsum and instead use np.cumsum/xp.cumulative_sum directly #31533

Open

OmarManzoor added 2 commits June 16, 2025 15:38

Merge branch 'main' into roc-curve

3666012

Use xp.cumulative_sum and add a non regression test for roc_curve pre…

37e64be

…cision with float32 weights

OmarManzoor added the CUDA CI label Jun 16, 2025

github-actions bot removed the CUDA CI label Jun 16, 2025

ogrisel approved these changes Jun 18, 2025

View reviewed changes

sklearn/metrics/_ranking.py Show resolved Hide resolved

sklearn/metrics/tests/test_ranking.py Outdated Show resolved Hide resolved

sklearn/metrics/tests/test_common.py Outdated Show resolved Hide resolved

OmarManzoor and others added 4 commits June 18, 2025 18:05

Update sklearn/metrics/_ranking.py

2ceae95

Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org>

Update sklearn/metrics/tests/test_common.py

70c5582

Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org>

Remove slow test

e3cea0f

Merge branch 'main' into roc-curve

dbdfbd5

OmarManzoor merged commit dab0842 into scikit-learn:main Jun 18, 2025
36 checks passed

jeremiedbb mentioned this pull request Jul 15, 2025

Release 1.7.1 #31762

Merged

13 tasks

Uh oh!

ENH: Make roc_curve array API compatible #30878

ENH: Make roc_curve array API compatible #30878

Conversation

lithomas1 commented Feb 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reference Issues/PRs

What does this implement/fix? Explain your changes.

Any other comments?

Uh oh!

github-actions bot commented Feb 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✔️ Linting Passed

Uh oh!

OmarManzoor left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

lithomas1 commented Mar 18, 2025

Uh oh!

OmarManzoor left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

OmarManzoor left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

lucyleeow Jun 5, 2025

Choose a reason for hiding this comment

Uh oh!

betatim Jun 5, 2025

Choose a reason for hiding this comment

Uh oh!

lucascolley Jun 5, 2025

Choose a reason for hiding this comment

Uh oh!

betatim Jun 5, 2025

Choose a reason for hiding this comment

Uh oh!

lucascolley Jun 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lucascolley Jun 5, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

lithomas1 commented Jun 5, 2025

Uh oh!

OmarManzoor commented Jun 5, 2025

Uh oh!

lithomas1 commented Jun 5, 2025

Uh oh!

OmarManzoor commented Jun 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

OmarManzoor commented Jun 11, 2025

Uh oh!

ogrisel commented Jun 12, 2025

Uh oh!

OmarManzoor commented Jun 12, 2025

Uh oh!

ogrisel commented Jun 13, 2025

Uh oh!

OmarManzoor commented Jun 16, 2025

Uh oh!

OmarManzoor commented Jun 16, 2025

lithomas1 commented Feb 22, 2025 •

edited

Loading

github-actions bot commented Feb 22, 2025 •

edited

Loading

OmarManzoor left a comment •

edited

Loading

lucascolley Jun 5, 2025 •

edited

Loading

OmarManzoor commented Jun 5, 2025 •

edited

Loading