API Rename force_all_finite into ensure_all_finite by jeremiedbb · Pull Request #29404 · scikit-learn/scikit-learn

jeremiedbb · 2024-07-03T14:18:50Z

renames force_all_finite into ensure_all_finite in the following public functions:

check_array
check_X_y
as_float_array
check_pairwise_arrays
pairwise_distances

The ensure_xxx pattern already used by several parameters of these functions only means that some checks are activated or not but has no effect on the output array.

force_all_finite follows this behavior but does not follow this naming pattern. I think it should, especially since we now have a parameter force_writeable that has an effect on the output array.

github-actions · 2024-07-03T14:20:12Z

✔️ Linting Passed

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

_{Generated for commit: 36d968e. Link to the linter CI: here}

thomasjpfan · 2024-07-09T00:58:38Z

sklearn/utils/validation.py

            f"{estimator_name} requires y to be passed, but the target y is None"
        )

+    if force_all_finite != "deprecated":


What should the behavior be when both are set?

check_array(force_all_finite=True, ensure_all_finite=False)

The way I implemented it for now is that if both are set, force_all_finite takes over.
I can see that we'd like to have an error if both are different though. Let me change that

Actually I'm not so sure. It makes the logic quite complicated.

What about the following ?
check_array(force_all_finite=False, ensure_all_finite=True)

ensure_all_finite=True is the default. The user already gets a warning to use ensure_all_finite instead of force_all_finite. I don't want to raise an additional error, while he just left the other to its default value. Otherwise we need a sentinel to be able to check if ensure_all_finite is set to a non-default value, but I don't think the added complexity is worth.

Instead, I added a comment in warning saying that ensure_all_finite is ignored when force_all_finite. I know that since ensure_all_finite is the new name it feels a bit weird and one would expect the opposite, but this way keeps things simple and avoids adding a complex logic.

What do you think ?

I'm +0.5 on just improving the docs/warning message.

The alternative is to have ensure_all_finite=None as the default that behaves as True. Then if force_all_finite!="deprecated" and ensure_all_finite is not None, raise an error that says that are not be set at the same time?

In two versions, we update ensure_all_finite=True and it'll be backward compatible because None behaved the same way.

The alternative is to have ensure_all_finite=None

Yes that's what I was not enthusiastic about. I would be in favor of doing that only if we agree to remove None as a valid option in 1.8 without deprecation 😄. Because otherwise I find that a 4 releases cycle is too long for a renaming.

Since this is a developer tools, I think I'm fine avoiding to have the default sentinel None and later change the default.

I'm not sure what @glemaitre is suggesting here. But I would rather use None, not document it, and later remove it after deprecation of force_...

I changed it so that now an error is raised when both are set. I added a comment to remove None in 1.8 without deprecation.

…nite

adrinjalali · 2024-07-18T14:26:28Z

sklearn/ensemble/_hist_gradient_boosting/tests/test_compare_lightgbm.py

+@pytest.mark.filterwarnings(
+    "ignore:'force_all_finite' was renamed to 'ensure_all_finite':FutureWarning"
+)


Is this coming from lightgbm?

Yes, technically from scikit-learn as a dependency of lightgbm.

adrinjalali · 2024-07-18T14:28:13Z

sklearn/metrics/pairwise.py

-        .. versionadded:: 0.22
+        .. versionadded:: 0.20


was this wrong?

No, just a copy paste mistake on my end. I fixed that.

adrinjalali · 2024-07-18T14:29:26Z

sklearn/metrics/pairwise.py

+        .. versionadded:: 0.20
+           Accepts the string ``'allow-nan'``.
+
+        .. versionchanged:: 0.23
+           Accepts `pd.NA` and converts it into `np.nan`
+
+        .. versionadded:: 1.6
+           `force_all_finite` was renamed to `ensure_all_finite`.


we only need a version added I guess?

The thing, if we don't copy it here, is that in 2 versions when we remove force_all_finite, then this information will disappear. I find it weird to remove the history for a renaming of a parameter.

I don't think this information is relevant anymore when we're renaming the parameter.

I agree with @adrinjalali. Basically since we change the name of parameter, we cannot go back to version 0.23 because this parameter is not existing there.

We just need to make sure that the options added are mentioned in the docstring that is the case here.

Ok I removed that

adrinjalali · 2024-07-18T14:32:39Z

sklearn/metrics/pairwise.py

+    if force_all_finite != "deprecated":
+        warnings.warn(
+            "'force_all_finite' was renamed to 'ensure_all_finite' in 1.6 and will be "
+            "removed in 1.8. Until then, ensure_all_finite is ignored when "
+            "force_all_finite is set.",
+            FutureWarning,
+        )


usually in these cases we raise if both are set. Only the new one should be set. More like

if force_all_finite != "deprecated" and ensure_all_finate != "default": raise ... elif force_all_finite != "deprecated": warn("use ensure...") ensure_... = force_... elif ensure_... == "default": ensure_... = True

See the discussion here #29404 (comment), and especially my last comment #29404 (comment). I agree with you, but I just want to make sure that we're on the same page regarding the removal of "default" or None (whichever we chose) in 1.8 without deprecation.

glemaitre

LGTM.

I considered to be slightly more lenient because we are touching to a developer tool. If it would be an estimator, I would go through the 4 deprecation cycles thingy.

…nite

thomasjpfan · 2024-07-25T14:13:34Z

sklearn/metrics/pairwise.py

+           `force_all_finite` was renamed to `ensure_all_finite` and will be removed
+           in 1.8.
+
+    ensure_all_finite : bool or 'allow-nan', default=True


I'm happy with the approach of using None as the default and documenting that it's True.

adrinjalali

Otherwise LGTM.

sklearn/utils/deprecation.py

Co-authored-by: Adrin Jalali <[email protected]>

rename force_all_finite into ensure_all_finite

35bf0c8

changelog

4432e70

jeremiedbb marked this pull request as ready for review July 3, 2024 14:25

jeremiedbb force-pushed the rename-force-all-finite branch from dacdf67 to 4432e70 Compare July 3, 2024 14:34

filter warnings in lightgbm compare test

7c0a41c

thomasjpfan reviewed Jul 9, 2024

View reviewed changes

jeremiedbb added 5 commits July 10, 2024 12:36

Merge remote-tracking branch 'upstream/main' into rename-force-all-fi…

9798256

…nite

add tests for deprecation messages

f06e914

fix test

ce2074c

more explicit warning

84987d9

update latest occurences

f7fdcfd

adrinjalali reviewed Jul 18, 2024

View reviewed changes

jeremiedbb added 2 commits July 18, 2024 18:51

cln bad copy paste

e0fdbf6

iter

f3a6756

glemaitre self-requested a review July 25, 2024 08:20

glemaitre approved these changes Jul 25, 2024

View reviewed changes

jeremiedbb added 3 commits July 25, 2024 15:01

Merge remote-tracking branch 'upstream/main' into rename-force-all-fi…

caa6a24

…nite

raise when both are set

c9dba75

iter

ab1d485

thomasjpfan approved these changes Jul 25, 2024

View reviewed changes

adrinjalali approved these changes Jul 25, 2024

View reviewed changes

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

Update sklearn/utils/deprecation.py

36d968e

Co-authored-by: Adrin Jalali <[email protected]>

adrinjalali merged commit 1c47967 into scikit-learn:main Jul 25, 2024

MarcBresson pushed a commit to MarcBresson/scikit-learn that referenced this pull request Sep 2, 2024

API Rename force_all_finite into ensure_all_finite (scikit-learn#29404)

ab7a1dd

Co-authored-by: Adrin Jalali <[email protected]>

ogrisel mentioned this pull request Mar 4, 2025

Partial dependence broken in sklearn 1.6.1 when grid has only two values #30938

Open

FrancoisPgm mentioned this pull request Oct 9, 2025

MNT Clean-up deprecations for 1.8: Rename force_all_finite to ensure_all_finite #32452

Merged

Uh oh!

Conversation

jeremiedbb commented Jul 3, 2024

Uh oh!

github-actions bot commented Jul 3, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✔️ Linting Passed

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jeremiedbb Jul 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

glemaitre left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adrinjalali left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

github-actions bot commented Jul 3, 2024 •

edited

Loading

jeremiedbb Jul 10, 2024 •

edited

Loading