FIX _check_reg_targets with Array API and multioutput argument #29143

Tialo · 2024-05-30T20:36:05Z

code to reproduce

from sklearn.metrics import mean_absolute_error
from sklearn import set_config

from array_api_strict import asarray

set_config(array_api_dispatch=True)

m = asarray([2, 3, 4])
t1 = asarray([[1., 2., 3.], [3., 3., 3]])
t2 = asarray([[1, 2, 10], [5, 2, 1]])

print(mean_absolute_error(t1, t2, multioutput=m))

traceback:

Traceback (most recent call last):
  File "/Users/y/Desktop/scikit-learn/chec_s.py", line 12, in <module>
    print(mean_absolute_error(t1, t2, multioutput=m))
  File "/Users/y/Desktop/scikit-learn/sklearn/utils/_param_validation.py", line 213, in wrapper
    return func(*args, **kwargs)
  File "/Users/y/Desktop/scikit-learn/sklearn/metrics/_regression.py", line 221, in mean_absolute_error
    _, y_true, y_pred, multioutput = _check_reg_targets(
  File "/Users/y/Desktop/scikit-learn/sklearn/metrics/_regression.py", line 142, in _check_reg_targets
    elif n_outputs != len(multioutput):
TypeError: object of type 'Array' has no len()

Reference Issues/PRs

What does this implement/fix? Explain your changes.

Any other comments?

Also, there is a function check_array_api_multioutput_regression_metric for testing regression multioutput metrics. But it only tests for multioutput=="raw_values", and not an array. Maybe array multioutput should also be tested?

github-actions · 2024-05-30T20:37:23Z

✔️ Linting Passed

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

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

ogrisel

LGTM.

@betatim @OmarManzoor any opinion on the need to add a non-regression test for such a specific fix?

OmarManzoor · 2024-06-07T13:37:05Z

Since mean_absolute_error has only been added in the current main branch I don't think it might be necessary to add a regression test but it might be useful to enhance the check_array_api_multioutput_regression_metric with multioutput==array

lesteve · 2024-06-17T08:30:19Z

sklearn/metrics/tests/test_common.py


-def check_array_api_multioutput_regression_metric(


Before, there was check_array_api_multioutput_regression_metric (multioutput in the middle) and check_array_api_regression_metric_multioutput (multioutput at the end), I merged both functions into one.

lesteve · 2024-06-17T08:30:43Z

sklearn/metrics/tests/test_common.py

@@ -1856,8 +1862,8 @@ def check_array_api_regression_metric(metric, array_namespace, device, dtype_nam
 def check_array_api_regression_metric_multioutput(
    metric, array_namespace, device, dtype_name
 ):
-    y_true_np = np.array([[1, 3], [1, 2]], dtype=dtype_name)
-    y_pred_np = np.array([[1, 4], [1, 1]], dtype=dtype_name)
+    y_true_np = np.array([[1, 3, 2], [1, 2, 2]], dtype=dtype_name)


I added more values to avoid having n_rows == n_columns

lesteve · 2024-06-17T08:31:16Z

I have added a test, I think this should make codecov happy. Edit: not quite since the exception case is still not covered. I think this should be dealt with in a separate PR. At least the behaviour is tested now.

…nto fix_check_reg_targets

lesteve · 2024-06-18T05:08:03Z

I have added a changelog, I added the PR in the already existing mean_absolute_error array API entry although I guess this may also have fixed other metrics, not 100% sure.

ogrisel · 2024-06-18T14:28:38Z

I launched the CUDA GPU CI at:

https://github.com/scikit-learn/scikit-learn/actions/runs/9566933791

ogrisel

Assuming green CI, LGTM with the latest changes.

OmarManzoor

Thanks @Tialo @lesteve. LGTM otherwise.

sklearn/metrics/tests/test_common.py

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

lesteve · 2024-06-20T08:55:11Z

OK merging the PR since there are two approvals, thanks @Tialo

Tialo added 2 commits May 30, 2024 23:30

fix

e74881d

remove f

ba4f351

github-actions bot added the module:metrics label May 30, 2024

Tialo changed the title ~~BUG _check_reg_targets breaks with Array API~~ FIX _check_reg_targets breaks with Array API May 31, 2024

ogrisel added the Array API label Jun 5, 2024

ogrisel approved these changes Jun 6, 2024

View reviewed changes

Add test

d68b63b

lesteve reviewed Jun 17, 2024

View reviewed changes

lesteve added 4 commits June 17, 2024 10:38

Merge branch 'main' of https://github.com/scikit-learn/scikit-learn i…

a1248ce

…nto fix_check_reg_targets

Fix after merge

83d5f7d

simplify test

54088bd

Add changelog

94aa797

lesteve changed the title ~~FIX _check_reg_targets breaks with Array API~~ FIX Fix _check_reg_targets with Array API and multioutput argument Jun 18, 2024

lesteve changed the title ~~FIX Fix _check_reg_targets with Array API and multioutput argument~~ FIX _check_reg_targets with Array API and multioutput argument Jun 18, 2024

ogrisel approved these changes Jun 18, 2024

View reviewed changes

OmarManzoor approved these changes Jun 20, 2024

View reviewed changes

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

lesteve and others added 2 commits June 20, 2024 09:33

Update sklearn/metrics/tests/test_common.py

1d7baf2

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

Further simplification

2535a13

lesteve merged commit e82c14b into scikit-learn:main Jun 20, 2024
30 checks passed

Tialo deleted the fix_check_reg_targets branch June 20, 2024 09:00

jeremiedbb mentioned this pull request Jul 2, 2024

Release 1.5.1 #29382

Merged

11 tasks

lesteve mentioned this pull request Oct 18, 2024

MAINT Tweak a few changelog entries #30102

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

FIX _check_reg_targets with Array API and multioutput argument #29143

FIX _check_reg_targets with Array API and multioutput argument #29143

Tialo commented May 30, 2024 •

edited

Loading

github-actions bot commented May 30, 2024 •

edited

Loading

ogrisel left a comment

OmarManzoor commented Jun 7, 2024 •

edited

Loading

lesteve Jun 17, 2024

lesteve Jun 17, 2024

lesteve commented Jun 17, 2024 •

edited

Loading

lesteve commented Jun 18, 2024

ogrisel commented Jun 18, 2024

ogrisel left a comment

OmarManzoor left a comment

lesteve commented Jun 20, 2024

FIX _check_reg_targets with Array API and multioutput argument #29143

FIX _check_reg_targets with Array API and multioutput argument #29143

Conversation

Tialo commented May 30, 2024 • edited Loading

Reference Issues/PRs

What does this implement/fix? Explain your changes.

Any other comments?

github-actions bot commented May 30, 2024 • edited Loading

✔️ Linting Passed

ogrisel left a comment

Choose a reason for hiding this comment

OmarManzoor commented Jun 7, 2024 • edited Loading

lesteve Jun 17, 2024

Choose a reason for hiding this comment

lesteve Jun 17, 2024

Choose a reason for hiding this comment

lesteve commented Jun 17, 2024 • edited Loading

lesteve commented Jun 18, 2024

ogrisel commented Jun 18, 2024

ogrisel left a comment

Choose a reason for hiding this comment

OmarManzoor left a comment

Choose a reason for hiding this comment

lesteve commented Jun 20, 2024

Tialo commented May 30, 2024 •

edited

Loading

github-actions bot commented May 30, 2024 •

edited

Loading

OmarManzoor commented Jun 7, 2024 •

edited

Loading

lesteve commented Jun 17, 2024 •

edited

Loading