Skip to content

ENH Add Array API compatibility to Binarizer - #31190

Merged
ogrisel merged 34 commits into
scikit-learn:mainfrom
lithomas1:array-api/binarizer
Apr 28, 2025
Merged

ENH Add Array API compatibility to Binarizer#31190
ogrisel merged 34 commits into
scikit-learn:mainfrom
lithomas1:array-api/binarizer

Conversation

@lithomas1

Copy link
Copy Markdown
Contributor

Reference Issues/PRs

Resubmit/rebase of #27588

What does this implement/fix? Explain your changes.

Any other comments?

@github-actions

github-actions Bot commented Apr 13, 2025

Copy link
Copy Markdown

✔️ Linting Passed

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

Generated for commit: 28eee37. Link to the linter CI: here

@lithomas1

Copy link
Copy Markdown
Contributor Author

@Tialo
I fixed up your old PR for making binarizer array API compatible if that's OK with you.

cc @OmarManzoor @ogrisel @betatim for review.

@lithomas1
lithomas1 marked this pull request as ready for review April 13, 2025 20:43
@lucyleeow

lucyleeow commented Apr 14, 2025

Copy link
Copy Markdown
Member

Thanks for the PR @lithomas1 , I think we may still be trying to reach consensus on this: #26024 (comment) , also linking related issue: #28668

(Edit: I think this may be relevant for your other PR #31191 as well)

@OmarManzoor

Copy link
Copy Markdown
Contributor

@lucyleeow I think we discussed that it wouldn't make sense to support the array api for LabelBinarizer. This is a different class simple Binarizer so shouldn't we move forward with reviewing this PR? Or is there some other point that I am missing?

@lucyleeow

Copy link
Copy Markdown
Member

@ogrisel may be better to clarify, but I am referring to this comment (#26024 (comment)):

Before opening any new PR on classification metrics (or maybe any metric), we should resolve the ongoing discussion in:

AFAICT the crux of this discussion is:

Similarly to the "y-follows-X" policy we want to implement in the fit method of estimators, we might want to decide officially on a y_true-follows-y_pred or y_pred-follows-y_true policy for the metric functions in case both inputs do not stem from the same namespace.

Though there is also a case for letting the user handle namespace changes. Regardless, I don't think there is consensus on this yet?

@OmarManzoor

Copy link
Copy Markdown
Contributor

Yes I think the comment you are referring to is related to LabelBinarizer.

Also for the metrics yes I think there is no consensus yet on how to handle the cases when y_pred and y_true are on different namespaces.

@lithomas1

Copy link
Copy Markdown
Contributor Author

Cool, do you know if the device issue is going to be brought up at the next array API meeting on Thursday?
(I'll try to listen in if so)

Similarly to the "y-follows-X" policy we want to implement in the fit method of estimators, we might want to decide
officially on a y_true-follows-y_pred or y_pred-follows-y_true policy for the metric functions in case both inputs do not stem from the same namespace.

Without having thought about this too much, I think y_true-follows-y_pred is probably the correct move here.
Assuming y_pred comes from some other estimator, it might be possible to overlap computation of y_pred with movement of y_true to the device.

I don't think this matters too much for metrics, though since they are cheap to compute.

@lucyleeow

lucyleeow commented Apr 17, 2025

Copy link
Copy Markdown
Member

I don't see an entry (in the agenda notes) for the next meeting (this Thurs), but maybe this is due to Easter holidays?

And I agree with you @OmarManzoor , I would love to see this work be able to move forward too. I'm just not clear on the status...

@ogrisel

ogrisel commented Apr 17, 2025

Copy link
Copy Markdown
Member

Array API namespaces can only represent numerical arrays. They cannot represent things like Python or NumPy string values or pandas categorical values: those dtypes are not part of the array API spec.

There is no point in adding array API support to LabelBinarizer because it is meant to transform non-numerical inputs (mostly string class labels used in y_train in fit or y_true in classification metrics) into numerical values (zeros and ones).

Binarizer on the other hand, is meant to transform numerical inputs into zeros and ones for a given choice of numerical threshold.

So it's perfectly meaningful to add array API support to it.

And as far as I understand it's unrelated to the y_true-follows-y_pred discussion which is blocking further work on classification metrics.

@ogrisel ogrisel left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Besides the following, LGTM.

Comment thread sklearn/preprocessing/_data.py Outdated
Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org>

@lucyleeow lucyleeow left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, just seems like a lint issue.

And as far as I understand it's unrelated to the y_true-follows-y_pred discussion which is blocking further work on classification metrics.

Thanks for clarifying @ogrisel , it just didn't register that this was not a metric. To clarify, should we be proceeding with support of classification metrics?

@ogrisel ogrisel moved this to In Progress in Array API Apr 23, 2025
@ogrisel

ogrisel commented Apr 23, 2025

Copy link
Copy Markdown
Member

To clarify, should we be proceeding with support of classification metrics?

Let's start with a documentation PR that explains the general policy to handle mixed namespace/devices inputs for metric functions (y_true follows y_pred).

@lithomas1

lithomas1 commented Apr 23, 2025

Copy link
Copy Markdown
Contributor Author

I fixed the lint issue.
Thanks for the reviews!

@ogrisel
ogrisel merged commit 39aaf13 into scikit-learn:main Apr 28, 2025
@github-project-automation github-project-automation Bot moved this from In Progress to Done in Array API Apr 28, 2025
@lithomas1
lithomas1 deleted the array-api/binarizer branch April 28, 2025 10:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

5 participants