[WIP] Add array-api support to metrics.confusion_matrix #28867
Conversation
| # import array_api_strict as xp | ||
| X = xp.asarray(X, copy=copy) | ||
| dtype = X.dtype | ||
| isscaler = X.ndim == 0 |
| msg = ( | ||
| "Cannot return indices with the torch backend yet. See" " array_api_compat." | ||
| ) | ||
| raise NotImplementedError(msg) |
There was a problem hiding this comment.
I have opened data-apis/array-api-compat#135. If array_api_compat maintainers accept this suggestion, then it might be worth contributing such a temporary workaround to array_api_compat. If not, we can implement our own temporary workaround for torch in scikit-learn.
| yield namespace, "cuda" | ||
| yield namespace, "mps" | ||
| else: | ||
| yield namespace, None |
There was a problem hiding this comment.
+1 for this. There once this is in, we should do a follow-up PR for occurrences of yield_namespace_device_dtype_combinations that discard the dtype value to avoid redundant test cases.
|
Please also add a changelog entry in |
| f"Got y_true={xp.unique(y_true)} and " | ||
| f"y_pred={xp.unique(y_pred)}. Make sure that the " | ||
| f"Got y_true={xp.unique_values(y_true)} and " | ||
| f"y_pred={xp.unique_values(y_pred)}. Make sure that the " | ||
| "predictions provided by the classifier coincides with " |
There was a problem hiding this comment.
| "predictions provided by the classifier coincides with " | |
| "predictions provided by the classifier coincide with " |
unrelated grammar fix (I think)
| if y_type not in ("binary", "multiclass"): | ||
| raise ValueError("%s is not supported" % y_type) | ||
|
|
||
| if labels is None: | ||
| labels = unique_labels(y_true, y_pred) | ||
| else: | ||
| labels = np.asarray(labels) | ||
| n_labels = labels.size | ||
| n_labels = size(labels) | ||
| if n_labels == 0: | ||
| raise ValueError("'labels' should contains at least one label.") |
There was a problem hiding this comment.
| raise ValueError("'labels' should contains at least one label.") | |
| raise ValueError("'labels' should contain at least one label.") |
unrelated typo fix
| @@ -542,14 +567,19 @@ def get_namespace(*arrays, remove_none=True, remove_types=(str,), xp=None): | |||
| # message in case it is missing. | |||
| import array_api_compat | |||
|
|
|||
| namespace, is_array_api_compliant = array_api_compat.get_namespace(*arrays), True | |||
| # Convert lists and tuple to numpy arrays. | |||
There was a problem hiding this comment.
Can we make the comment so it explains why it is a good idea to do this? I think that would be helpful to have here, at least for me it is not 100% clear that we should do this
| if size(y_true) == 0: | ||
| return xp.zeros((n_labels, n_labels), dtype=xp.int32, device=device) | ||
|
|
||
| if size(_intersect1d(y_true, labels, xp=xp)) == 0: |
There was a problem hiding this comment.
What happens here if labels is a Python list of strings and y_true is, say, a torch array?
As a user I'd probably provide the labels as a Python list/tuple, mostly because it is convenient and not performance critical.
Is there a downside to being helpful to the callers and allowing list/tuple here?
|
I am going to close this one since there has been some work in the meantime on Hopefully you don't mind too much @charlesjhill 🙏. |
|
Sorry @charlesjhill, that was an oversight. I wasn't aware you had opened a WIP PR before. |
Reference Issues/PRs
See #26024 for the array-api meta-issue tracking the "tools" in sklearn.
What does this implement/fix? Explain your changes.
This PR adds array-api compatibility to the
sklearn.metrics.confusion_matrixmethod, aiming to support all of its current API surface. Many other classification metrics are or can be computed based on a confusion matrix so it seems fairly high value to port.TODO:
Any other comments?
None for now :)