✔️ Linting Passed

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

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

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.

I've updated the PR.

Since #30613 is merged, we don't need the _convert_pandas_nullable_dtypes() helper function anymore.

Edit: Oh my, we still get the same CI error:

FAILED metrics/tests/test_classification.py::test_confusion_matrix_pandas_nullable[Int64] - ValueError: Classification metrics can't handle a mix of unknown and binary...
FAILED metrics/tests/test_classification.py::test_confusion_matrix_pandas_nullable[Float64] - ValueError: Classification metrics can't handle a mix of unknown and binary...
FAILED metrics/tests/test_classification.py::test_confusion_matrix_pandas_nullable[boolean] - ValueError: Classification metrics can't handle a mix of unknown and binary...
= 3 failed, 32952 passed, 3142 skipped, 146 xfailed, 70 xpassed, 4114 warnings in 1138.31s (0:18:58) =

So, should we:
a) re-introduce the _convert_pandas_nullable_dtypes() helper function?
b) call check array() from within _convert_to_numpy() to fix this? Edit: A few weeks later now I have come to the conclusion that the way check_array needs to be called is very specific to the data we expect and then maybe it's better not to call it from within a function that is going to be used so so many different places, but rather right after user input. So I don't think that b) is a good idea anymore.

In check_array() these issues were dealt with in #25814 and in #25147 in a more general way.

I think the second option is the more secure, because the helper function fixes only this specific test, but check_array()can deal with all the pandas extension dtypes, which involve more if I understand correctly.

@ogrisel: you surely have an intuition here?

Edit: Oh my, we still get the same CI error:

FAILED metrics/tests/test_classification.py::test_confusion_matrix_pandas_nullable[Int64] - ValueError: Classification metrics can't handle a mix of unknown and binary...
FAILED metrics/tests/test_classification.py::test_confusion_matrix_pandas_nullable[Float64] - ValueError: Classification metrics can't handle a mix of unknown and binary...
FAILED metrics/tests/test_classification.py::test_confusion_matrix_pandas_nullable[boolean] - ValueError: Classification metrics can't handle a mix of unknown and binary...
= 3 failed, 32952 passed, 3142 skipped, 146 xfailed, 70 xpassed, 4114 warnings in 1138.31s (0:18:58) =

I couldn't reproduce the CI error mentioned above on my local machine. In fact, test_confusion_matrix_pandas_nullable passed in all three cases:

Could this be a false positive?

Thanks for checking this @virchan.

I have now re-tried, runing pytest on an environment created from the lockfile for the min dependencies.
(conda create --name min_dep_env --file ./build_tools/azure/pymin_conda_forge_openblas_min_dependencies_linux-64_conda.lock).

I now get the failures locally too.

When I tried before, the test has passed locally with pandas==1.2.0. However, @lesteve pointed me to the possibility that there might be some interplay between dependencies that causes this. I'm actually not sure anymore how I did run the test. Maybe I wasn't in my min_dep_env and instead only lazily re-installed my pandas with numpy unaltered. So this might have been the reason why I had seen these tests pass locally.

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?

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?).

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_)

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

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?

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)?

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

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.

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