ENH Add Array API compatibility to MinMaxScaler #26243

betatim · 2023-04-21T13:12:27Z

Reference Issues/PRs

What does this implement/fix? Explain your changes.

This enables MinMaxScaler to work with Array API compatible arrays. Most of the changes are replacing np with xp (which represents the namespace the array the user passed in belongs to). Had to implement some helpers like nanmin and nanmax.

Any other comments?

thomasjpfan

This looks fairly straight forward. We'll need tests to make sure PyTorch works on the CI and gives correct results.

For completeness, inverse_transform can also be updated with array_api.

sklearn/utils/_array_api.py

betatim · 2023-04-26T12:06:09Z

Done and done.

For the tests I more or less took the tests that exist for LinearDiscriminantAnalysis. They are quite similar but not identical. I was thinking I'll make a new PR that tries to make a set of "common tests". Probably via adding a new estimator tag that can be used to discover things that support the array API. WDYT?

sklearn/preprocessing/_data.py

thomasjpfan

I am uneasy about supporting xp.float16. Adding a float16 property to _ArrayAPIWrapper feels like we are "extending the Array API spec".

My preference is to be strict and only allow float16 when the arrays are ndarray. This means we do not support float16 for numpy.array_api Arrays, torch, cupy.array_api, etc.

sklearn/utils/_array_api.py

sklearn/preprocessing/_data.py

betatim · 2023-05-02T13:55:34Z

My preference is to be strict and only allow float16 when the arrays are ndarray. This means we do not support float16 for numpy.array_api Arrays, torch, cupy.array_api, etc.

Fine for me. Though as far as I can see numpy.array_api and cupy.array_api will be the only namespaces for which we won't have float16. For cupy, torch and numpy it exists.

thomasjpfan

Okay, I am mostly onboard with supporting float16.

sklearn/utils/_array_api.py

ogrisel

Besides the following and @thomasjpfan's comment on isneginf, LGTM.

Not sure how to efficiently cope with the lack of a generic isneginf though.

sklearn/preprocessing/tests/test_data.py

sklearn/utils/_array_api.py

ogrisel

Beyond making atol stricter in the tests and the following suggestions to improve comments / docstrings, LGTM!

sklearn/utils/_array_api.py

sklearn/preprocessing/tests/test_data.py

sklearn/utils/_array_api.py

betatim · 2023-06-01T09:23:37Z

sklearn/utils/_array_api.py

@@ -357,6 +368,35 @@ def _expit(X):
    return 1.0 / (1.0 + xp.exp(-X))


+def _isneginf(X):


TODO for Tim: open issue in Array API about asymmetry between isinf and isneginf

ogrisel · 2023-06-14T09:41:48Z

I think this PR needs to be updated to leverage the new common test recently merged in main.

github-actions · 2023-06-29T12:17:26Z

✔️ Linting Passed

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

_{Generated for commit: 54e7f5f. Link to the linter CI: here}

betatim · 2023-07-04T11:45:12Z

I think this is ready to go now. I'd like to add more tests a la:

@parametrize_with_checks(
    [
        MinMaxScaler()
    ],
    check_yielder=_yield_array_api_checks,
)
def test_array_api_compliance(check, estimator, request):
    check(estimator, check_values=True)

but this uses things from #26315. Either we wait to merge this PR until #26315 has landed or we make another PR later to add those tests. I'm fine with either. Maybe waiting for #26315 is cleaner/requires less bookkeeping work.

ogrisel · 2023-07-06T15:59:03Z

I just reverted the change made to parametrize_with_checks because in the end I no longer needed it in #26315. Instead I used a cross-product of regular pytest.mark.parametrize:

https://github.com/scikit-learn/scikit-learn/pull/26315/files#diff-7fdbfa7b6564912adf698653f0faa9248c7ad600cdccf607451b5c500dbf6b4cR731-R755

Note: the order of the decorators matters for the name of the tests.

ogrisel

Thanks for the update. Still LGTM.

sklearn/preprocessing/tests/test_data.py

betatim · 2023-07-20T08:36:20Z

@thomasjpfan what do you think?

thomasjpfan

Can you add the estimator to:

scikit-learn/doc/modules/array_api.rst

Lines 88 to 89 in 2b0eef8

    
           Estimators with support for `Array API`-compatible inputs 
        
           =========================================================

I ran this on my local machine with CuPy + PyTorch and the new tests pass.

doc/whats_new/v1.3.rst

sklearn/utils/_array_api.py

When using the axis keyword it can happen that a slice contains only NaNs. This change corrects the logic that restores the NaNs at the end. Added tests for 2D inputs.

thomasjpfan

Minor comment, otherwise LGTM

sklearn/utils/_array_api.py

…ray-api

Co-authored-by: Thomas J. Fan <thomasjpfan@gmail.com>

github-actions bot added module:preprocessing module:utils labels Apr 21, 2023

thomasjpfan reviewed Apr 25, 2023

View reviewed changes

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

betatim marked this pull request as ready for review April 26, 2023 12:02

betatim commented Apr 27, 2023

View reviewed changes

sklearn/preprocessing/_data.py Outdated Show resolved Hide resolved

thomasjpfan reviewed Apr 28, 2023

View reviewed changes

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

sklearn/preprocessing/_data.py Outdated Show resolved Hide resolved

betatim added the Array API label May 4, 2023

ogrisel added the Waiting for Reviewer label May 4, 2023

thomasjpfan reviewed May 4, 2023

View reviewed changes

sklearn/utils/_array_api.py Show resolved Hide resolved

sklearn/utils/_array_api.py Show resolved Hide resolved

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

ogrisel mentioned this pull request May 5, 2023

ENH Array API support for PCA #26315

Merged

7 tasks

ogrisel reviewed May 5, 2023

View reviewed changes

sklearn/preprocessing/tests/test_data.py Outdated Show resolved Hide resolved

sklearn/utils/_array_api.py Show resolved Hide resolved

This was referenced May 8, 2023

Add common tests for estimators that support the Array API #26348

Closed

ENH Add common Array API tests and estimator tag #26372

Merged

ogrisel approved these changes May 22, 2023

View reviewed changes

betatim commented Jun 1, 2023

View reviewed changes

betatim force-pushed the minmax-array-api branch from f28eab5 to 8e8e965 Compare June 29, 2023 12:15

betatim force-pushed the minmax-array-api branch from 547f487 to 2d690a7 Compare July 14, 2023 09:49

ogrisel approved these changes Jul 18, 2023

View reviewed changes

sklearn/preprocessing/tests/test_data.py Outdated Show resolved Hide resolved

betatim added Waiting for Second Reviewer First reviewer is done, need a second one! and removed Waiting for Reviewer labels Jul 20, 2023

thomasjpfan reviewed Jul 29, 2023

View reviewed changes

doc/whats_new/v1.3.rst Outdated Show resolved Hide resolved

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

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

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

betatim added 2 commits August 4, 2023 16:40

Add Array API compatibility to MinMaxScaler

5e27be2

Ensure returned arrays are in the same namespace

53964be

betatim added 13 commits August 4, 2023 16:40

Don't add float16 to Numpy Array API namespace

3eea897

Use our own isneginf function

a6939a4

Add float16 to Numpy wrapper, add float dtype test

e5a0572

Ruffing it up

02d89d7

Fix author

9d5b701

Add tests for NaN reductions

c89948f

Add comments about special status of float16

6bb0ad5

Change library order

77af8a6

Enable array API when testing

daf9adc

Formatting

e0e9102

Fix typo

c7ade93

Move changelog entry

89b0cf8

Fix all NaN slice handling

beb130a

When using the axis keyword it can happen that a slice contains only NaNs. This change corrects the logic that restores the NaNs at the end. Added tests for 2D inputs.

betatim force-pushed the minmax-array-api branch from c9bbd4c to beb130a Compare August 4, 2023 14:43

Remove isneginf() as it is unused

775868e

thomasjpfan approved these changes Aug 7, 2023

View reviewed changes

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

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

thomasjpfan changed the title ~~Add Array API compatibility to MinMaxScaler~~ ENH Add Array API compatibility to MinMaxScaler Aug 7, 2023

betatim mentioned this pull request Aug 18, 2023

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

Open

betatim and others added 3 commits August 18, 2023 10:35

Change order to avoid having to create a duplicate mask

5001892

Merge remote-tracking branch 'upstream/main' into minmax-array-api

338c0d7

Merge remote-tracking branch 'upstream/main' into pr/26243

dba261d

thomasjpfan enabled auto-merge (squash) August 18, 2023 12:47

betatim added 2 commits August 18, 2023 15:33

Empty-Commit

dc2a40d

Merge remote-tracking branch 'origin/minmax-array-api' into minmax-ar…

54e7f5f

…ray-api

thomasjpfan merged commit 061f877 into scikit-learn:main Aug 18, 2023

betatim deleted the minmax-array-api branch August 18, 2023 15:35

ogrisel mentioned this pull request Aug 21, 2023

Make standard scaler compatible to Array API #27113

Open

TamaraAtanasoska pushed a commit to TamaraAtanasoska/scikit-learn that referenced this pull request Aug 21, 2023

ENH Add Array API compatibility to MinMaxScaler (scikit-learn#26243)

45f5ced

Co-authored-by: Thomas J. Fan <thomasjpfan@gmail.com>

akaashpatelmns pushed a commit to akaashp2000/scikit-learn that referenced this pull request Aug 25, 2023

ENH Add Array API compatibility to MinMaxScaler (scikit-learn#26243)

edc8a10

Co-authored-by: Thomas J. Fan <thomasjpfan@gmail.com>

REDVM pushed a commit to REDVM/scikit-learn that referenced this pull request Nov 16, 2023

ENH Add Array API compatibility to MinMaxScaler (scikit-learn#26243)

941a474

Co-authored-by: Thomas J. Fan <thomasjpfan@gmail.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ENH Add Array API compatibility to MinMaxScaler #26243

ENH Add Array API compatibility to MinMaxScaler #26243

betatim commented Apr 21, 2023

thomasjpfan left a comment

betatim commented Apr 26, 2023

thomasjpfan left a comment

betatim commented May 2, 2023

thomasjpfan left a comment

ogrisel left a comment

ogrisel left a comment

betatim Jun 1, 2023

ogrisel commented Jun 14, 2023

github-actions bot commented Jun 29, 2023 •

edited

Loading

betatim commented Jul 4, 2023 •

edited

Loading

ogrisel commented Jul 6, 2023 •

edited

Loading

ogrisel left a comment

betatim commented Jul 20, 2023

thomasjpfan left a comment

thomasjpfan left a comment

		@@ -357,6 +368,35 @@ def _expit(X):
		return 1.0 / (1.0 + xp.exp(-X))


		def _isneginf(X):

	Estimators with support for `Array API`-compatible inputs
	=========================================================

ENH Add Array API compatibility to MinMaxScaler #26243

ENH Add Array API compatibility to MinMaxScaler #26243

Conversation

betatim commented Apr 21, 2023

Reference Issues/PRs

What does this implement/fix? Explain your changes.

Any other comments?

thomasjpfan left a comment

Choose a reason for hiding this comment

betatim commented Apr 26, 2023

thomasjpfan left a comment

Choose a reason for hiding this comment

betatim commented May 2, 2023

thomasjpfan left a comment

Choose a reason for hiding this comment

ogrisel left a comment

Choose a reason for hiding this comment

ogrisel left a comment

Choose a reason for hiding this comment

betatim Jun 1, 2023

Choose a reason for hiding this comment

ogrisel commented Jun 14, 2023

github-actions bot commented Jun 29, 2023 • edited Loading

✔️ Linting Passed

betatim commented Jul 4, 2023 • edited Loading

ogrisel commented Jul 6, 2023 • edited Loading

ogrisel left a comment

Choose a reason for hiding this comment

betatim commented Jul 20, 2023

thomasjpfan left a comment

Choose a reason for hiding this comment

thomasjpfan left a comment

Choose a reason for hiding this comment

github-actions bot commented Jun 29, 2023 •

edited

Loading

betatim commented Jul 4, 2023 •

edited

Loading

ogrisel commented Jul 6, 2023 •

edited

Loading