ENH Array API support for confusion_matrix #30562

StefanieSenger · 2024-12-30T09:49:34Z

Reference Issues/PRs

towards #26024
closes #30440 (supercedes)

This PR is an alternative, discussed in #30440. It accepts array inputs from all namespaces, converts the input arrays to numpy arrays right away to do the calculations in numpy (which is necessary for the coo_matrix at least) and returns the confusion_matrix in the same namespace as the input and on a cpu device.

That's what we had discussed. For more details see the discussions on both PRs.

github-actions · 2024-12-30T09:50:53Z

✔️ Linting Passed

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

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

sklearn/utils/_array_api.py

StefanieSenger · 2024-12-30T11:31:42Z

sklearn/metrics/tests/test_classification.py

+        result = confusion_matrix(y_true, y_pred)
+        xp_result, _ = get_namespace(result)
+        assert _is_numpy_namespace(xp_result)
+
+        # Since the computation always happens with NumPy / SciPy on the CPU, this
+        # function is expected to return an array allocated on the CPU even when it does
+        # not match the input array's device.
+        assert result.device == "cpu"


I have adjusted this test to your suggestions from this comment, @ogrisel. But here, the test is narrower because we made the return value of confusion_matrix to always be a numpy array on cpu.

StefanieSenger · 2024-12-30T11:50:08Z

Regarding the question how to document the return type of confusion_matrix() as a numpy array, I think that keeping
C : ndarray of shape (n_classes, n_classes) in the docstring should be enough, assumed that all the other functions and methods where we have added array api support document the return value type correctly, which is currently not the case.

sklearn/utils/_array_api.py

virchan

I suspect the CI failure (about the Test Library) is a false positive, as I couldn't reproduce the same error after running the tests multiple times on my local machine. Instead, I encountered a different set of errors related to CUDA. I believe the issue might be linked to how we convert sample_weight into a NumPy array.

sklearn/metrics/_classification.py

OmarManzoor

I think this looks good overall. Thanks @StefanieSenger
We just need to finalize whether we want to keep the current flow where we return a numpy array on the cpu or should we convert to the initial namespace and device.
CC: @ogrisel

sklearn/metrics/_classification.py

sklearn/metrics/tests/test_classification.py

Co-authored-by: Omar Salman <omar.salman2007@gmail.com>

sklearn/metrics/_classification.py

lesteve · 2025-02-11T06:08:45Z

We just need to finalize whether we want to keep the current flow where we return a numpy array on the cpu or should we convert to the initial namespace and device.

About namespace, I would say for now the least controversial thing to do (from the point of view of array API support) would be to convert to the initial namespace, for example see #30440 (comment).

About device, I am not quite sure ... the Scipy array API support blog post compiled code section (from October 2023) says:

or now, we attempt to convert to NumPy using np.asarray before the compiled code, then convert back to our array's namespace using xp.asarray after it. This will raise exceptions for arrays on a different device to the CPU, as justified in the RFC (tldr: we want to avoid silent device transfers).

Maybe @lucascolley has some opinion on what to do with device based on his scipy experience?

lucascolley · 2025-02-11T07:20:19Z

thanks for the ping @lesteve, I left my initial thoughts at #30440 (comment) then discussed this with Olivier and Tim in a call. The summary is that I think you'll want to think carefully before setting the precedent for different defaults across functions as to whether they return the input namespace or NumPy. That seems like it could be confusing for users and difficult to document.

In this case, if (some of) the computation has to be done with NumPy, then I would suggest attempting to convert back to the input namespace. In SciPy we have chosen to let this error for now and skip the tests for device transfers, but you could choose to let the device transfers happen (as I think you do in your _convert_to_numpy, right?).

OmarManzoor · 2025-02-16T16:52:44Z

I ran some benchmarks on a Kaggle kernel with CUDA using this branch where in one case, the current flow in which a numpy array is returned is used, while in the other we convert to the namespace and device when returning return xp.asarray(cm, device=device_)

Array Size (CUDA)	Return Numpy Timing	Return Namespace and Device Timing
1e3	0.002571168	0.002800589
1e4	0.005218287	0.005717518
1e5	0.010124249	0.011052353
1e6	0.041873491	0.044407668
1e7	0.571530447	0.586297677

By looking at this, I think we might as well return the confusion matrix in the original namespace and device.

StefanieSenger · 2025-02-16T21:01:39Z

Thanks, everyone, for your thoughts on the return type!

To summarize:

The original decision (based on @ogrisel’s suggestion) was to default to returning numpy arrays.
@OmarManzoor’s benchmarks show that converting back to the original namespace and device is a little less performant than keeping the arrays numpy arrays, but the difference is small.
@lesteve and @lucascolley brought up the user experience and consistency across functions and users get the return types that they expect.

So, should we finalise the PR by converting back to the input namespace and device, despite the minor performance difference? Or should we keep the numpy default?

I believe the advantages of converting back to the input namespace and device overweigh and in case we would later judge differently for some reason, array API is experimental and we could still change its behaviour without annoying users too much.

@ogrisel, since your suggestion was to convert to numpy, do you have any thoughts on whether we should stick with that or return types from the input namespace based on the discussion?

lucascolley · 2025-02-16T21:21:06Z

So, should we finalise the PR by converting back to the input namespace and device, despite the minor performance difference?

+1 from me!

For clarity, what is the NumPy code (i.e. which lines) that we have been unable to write in a performant way with the standard (which caused the initial performance discussion)?

OmarManzoor · 2025-02-18T06:18:55Z

For clarity, what is the NumPy code (i.e. which lines) that we have been unable to write in a performant way with the standard (which caused the initial performance discussion)?

Basically where we create the confusion matrix using a coo_matrix

scikit-learn/sklearn/metrics/_classification.py

Lines 413 to 417 in 869f568

    
           cm = coo_matrix( 
        
               (sample_weight, (y_true, y_pred)), 
        
               shape=(n_labels, n_labels), 
        
               dtype=dtype, 
        
           ).toarray()

lesteve · 2025-02-18T09:08:14Z

So, should we finalise the PR by converting back to the input namespace and device, despite the minor performance difference?

This seems the less surprising to me for now and we can revisit this later if the need arises. I would think metrics computation is unlikely to be the bottleneck in most real life use cases.

OmarManzoor · 2025-03-13T06:52:53Z

@ogrisel Could you kindly provide your feedback so that we can finalize this PR?

ogrisel · 2025-06-19T06:56:34Z

Based on the discussion in #31286 (comment), the decision is to convert such output arrays back to the original namespace.

OmarManzoor

Thank you for the updates @StefanieSenger

OmarManzoor · 2025-07-15T13:04:29Z

doc/modules/array_api.rst

@@ -141,6 +141,7 @@ Metrics
 -------

 - :func:`sklearn.metrics.accuracy_score`
+- :func:`sklearn.metrics.confusion_matrix`


I think we should have another section where we specify metrics that don't support the array api in internal computations and use numpy instead because the array api does not support the optimizations needed. Therefore using numpy is more appropriate internally and we convert the output back to the desired array.
This should be done because we don't guarantee and speedups for such metrics.

I understand, @OmarManzoor. But metrics computation is not the bottleneck for performance and it is hard to know where to draw the line, because confusion_matrix is internally used in so many other functions and methods. So, I am not so sure if we can decide on a good way to document that on this PR. Should we maybe rather discuss that somewhere else?

I think it's fine to not add the additional document in this PR. But don't you think that where other metrics that are documented use the array api internally and the ones involving confusion matrix or confusion matrix itself don't use the array api, it would be better to clarify such a point? I think we can ask for opinions from others as well.

Thanks for understanding and your support on this PR, @OmarManzoor!
I don't mind either way and am happy to support the docs moving forward so or so.

If you would ask for my opinion (but it's really not deciding), then I would tend to say we don't need to justify in detail what happens internally at the moment. I would consider confusion_matrix and any other function that doesn't break when we use other array libraries on it now to be array api compatible. You can feed it arrays of any type and it returns arrays from the same type and thus it's compatible to be used as part of a data science pipeline.

For metrics the speedup by actually staying on gpu is not so high. And there will be many other places where we cannot support staying on gpu (yet) for different reasons, and that might change over time as array api evolves or we find better solutions. I think it is very ambitious to try to document internal details in such an early state of implementing array api in experimental state.

By the way the main reason for getting other opinions is that this was suggested previously #29881 (comment)

sklearn/metrics/_classification.py

StefanieSenger

Thanks for your reviews, @OmarManzoor and @lesteve!
I have applied your suggestions and commented regarding the documentation.

Now, this PR looks very straightforward. :)

StefanieSenger · 2025-07-16T09:55:31Z

doc/modules/array_api.rst

@@ -141,6 +141,7 @@ Metrics
 -------

 - :func:`sklearn.metrics.accuracy_score`
+- :func:`sklearn.metrics.confusion_matrix`


I understand, @OmarManzoor. But metrics computation is not the bottleneck for performance and it is hard to know where to draw the line, because confusion_matrix is internally used in so many other functions and methods. So, I am not so sure if we can decide on a good way to document that on this PR. Should we maybe rather discuss that somewhere else?

StefanieSenger and others added 14 commits December 7, 2024 23:51

ENH Array API for confusion_matrix

78d2a65

fix dtype checking

770e638

prepare for PR

af440ca

change log

b45646e

use our _isin

3db7054

changes after review

abab5ea

forgot to push that before

abc3981

add test

09cec5d

fix sclar dtype

fdb25f6

fix typos

49f75b7

convert_to_numpy and coo_matrix instead of python loop

914bb63

Merge branch 'main' into array_api_confusion_matrix

a939c80

experiment with convert_to_numpy

6da1d06

np.intersect1d can stay as it is

1f23f63

github-actions bot added module:metrics module:utils labels Dec 30, 2024

StefanieSenger added 2 commits December 30, 2024 10:55

return cm as numpy array

6a43bc3

move attach unique to after conversion to numpy

2000a00

StefanieSenger commented Dec 30, 2024

View reviewed changes

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

adjust test

5963e0f

StefanieSenger commented Dec 30, 2024

View reviewed changes

document return array type

ef84e04

StefanieSenger requested a review from ogrisel December 30, 2024 11:50

StefanieSenger added 2 commits January 2, 2025 09:36

use get_namespace

1cf525e

fix issue with nullable dtypes with pandas==1.1.5

f50f3ea

StefanieSenger commented Jan 2, 2025

View reviewed changes

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

private function

84038e4

virchan reviewed Jan 3, 2025

View reviewed changes

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

StefanieSenger and others added 5 commits January 31, 2025 12:12

add label passing to test to archive CodeCov

32ea61e

fix naming

e59cd7f

experiment - need to push so I can test on GPU

5124f98

convert labels to numpy

5034d01

Merge branch 'main' into array_api_confusion_matrix_numpy

3cffcbf

OmarManzoor reviewed Feb 10, 2025

View reviewed changes

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

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

StefanieSenger and others added 3 commits February 10, 2025 12:28

Update sklearn/metrics/tests/test_classification.py

d42caa6

Co-authored-by: Omar Salman <omar.salman2007@gmail.com>

remove unhelpful comment

094ca6d

Merge branch 'main' into array_api_confusion_matrix_numpy

869f568

virchan reviewed Feb 11, 2025

View reviewed changes

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

StefanieSenger mentioned this pull request May 5, 2025

Clarification of output array type when metrics accept multiclass/multioutput #31286

Closed

lucyleeow mentioned this pull request Jun 26, 2025

Make more of the "tools" of scikit-learn Array API compatible #26024

Open

StefanieSenger and others added 2 commits July 15, 2025 12:46

Merge branch 'main' into array_api_confusion_matrix_numpy

6d23419

convert back to original namespace and keep cpu device

2edcf8e

StefanieSenger changed the title ~~ENH Array API support for confusion_matrix converting to numpy array~~ ENH Array API support for confusion_matrix Jul 15, 2025

OmarManzoor reviewed Jul 15, 2025

View reviewed changes

changes after review

a5bf169

StefanieSenger added the CUDA CI label Jul 16, 2025

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

StefanieSenger commented Jul 16, 2025

View reviewed changes

Merge branch 'main' into array_api_confusion_matrix_numpy

725cb24

Uh oh!

ENH Array API support for confusion_matrix #30562

Are you sure you want to change the base?

ENH Array API support for confusion_matrix #30562

Uh oh!

Conversation

StefanieSenger commented Dec 30, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reference Issues/PRs

Uh oh!

github-actions bot commented Dec 30, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✔️ Linting Passed

Uh oh!

Uh oh!

StefanieSenger Dec 30, 2024

Choose a reason for hiding this comment

Uh oh!

StefanieSenger commented Dec 30, 2024

Uh oh!

Uh oh!

virchan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

OmarManzoor left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

lesteve commented Feb 11, 2025

Uh oh!

lucascolley commented Feb 11, 2025

Uh oh!

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

Uh oh!

Uh oh!

StefanieSenger commented Feb 16, 2025

Uh oh!

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

Uh oh!

Uh oh!

OmarManzoor commented Feb 18, 2025

Uh oh!

lesteve commented Feb 18, 2025

Uh oh!

OmarManzoor commented Mar 13, 2025

Uh oh!

ogrisel commented Jun 19, 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!

OmarManzoor Jul 15, 2025

Choose a reason for hiding this comment

Uh oh!

StefanieSenger Jul 16, 2025

Choose a reason for hiding this comment

Uh oh!

OmarManzoor Jul 17, 2025

Choose a reason for hiding this comment

Uh oh!

StefanieSenger Jul 17, 2025

Choose a reason for hiding this comment

Uh oh!

OmarManzoor Jul 17, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

StefanieSenger left a comment

Choose a reason for hiding this comment

Uh oh!

StefanieSenger Jul 16, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

StefanieSenger commented Dec 30, 2024 •

edited

Loading

github-actions bot commented Dec 30, 2024 •

edited

Loading

OmarManzoor commented Feb 16, 2025 •

edited

Loading

lucascolley commented Feb 16, 2025 •

edited

Loading

OmarManzoor left a comment •

edited

Loading