MAINT Param validation: apply skip nested validation to all functions #26495

jeremiedbb · 2023-06-02T17:16:49Z

Follow-up of #25815
This PR sets prefer_skip_nested_validation to all validated functions.

There are situations where we don't want to skip inner validation (prefer_skip_nested_validation=False):

the user passes an unfitted estimator instance
the user passes a callable and the args for the callable as a dict (e.g. metric and metric_params). Note that if the user only passes a callable we want to skip inner validation because the args passed to the callable come from us and not from the user.
the function is just a wrapper around an estimator class and hence only performs partial validation.

Functions receiving cv objects can skip inner validation because cv objects are not validated yet. When we decide to validate cv objects we'll need to revisit this.

adrinjalali · 2023-06-06T10:23:41Z

Should we then make prefer_skip_nested_validation a required arg, so that we always set it and think about how to set it?

jeremiedbb · 2023-06-06T12:05:55Z

That's what I did :) see the new signature:
def validate_params(parameter_constraints, *, prefer_skip_nested_validation):

adrinjalali

Do you think it'd be feasible for you to put a comment wherever prefer_skip_nested_validation=False? Cause it's almost always True

adrinjalali · 2023-06-06T12:13:12Z

sklearn/cluster/_optics.py

@@ -680,7 +681,8 @@ def _set_reach_dist(
        "core_distances": [np.ndarray],
        "ordering": [np.ndarray],
        "eps": [Interval(Real, 0, None, closed="both")],
-    }
+    },
+    prefer_skip_nested_validation=True,


why is this here True but compute_optics_graph has it as False?

jeremiedbb

@adrinjalali here's a list of all places where I set it to False with the reason for each.

sklearn/cluster/_affinity_propagation.py

sklearn/cluster/_kmeans.py

sklearn/cluster/_mean_shift.py

sklearn/cluster/_spectral.py

sklearn/covariance/_graph_lasso.py

sklearn/metrics/pairwise.py

sklearn/model_selection/_validation.py

jeremiedbb

@adrinjalali here's a list of all places where I set it to False with the reason for each.

adrinjalali

For the ones which are False and the reason is something other than them being an estimator wrapper, it'd be nice to have the reason commented in the code for future maintainers.

adrinjalali · 2023-06-09T10:47:03Z

sklearn/preprocessing/_data.py

@@ -559,7 +560,8 @@ def _more_tags(self):
    {
        "X": ["array-like"],
        "axis": [Options(Integral, {0, 1})],
-    }
+    },
+    prefer_skip_nested_validation=False,


why is this false but the other ones in this file True?

adrinjalali · 2023-06-09T10:49:57Z

sklearn/tests/test_public_functions.py

@@ -296,6 +296,7 @@ def test_function_param_validation(func_module):

 PARAM_VALIDATION_CLASS_WRAPPER_LIST = [


minmax_scale and similar ones are not?

adrinjalali · 2023-06-14T12:51:14Z

sklearn/tests/test_public_functions.py

@@ -305,6 +304,7 @@ def test_function_param_validation(func_module):
    ("sklearn.decomposition.dict_learning", "sklearn.decomposition.DictionaryLearning"),
    ("sklearn.decomposition.fastica", "sklearn.decomposition.FastICA"),
    ("sklearn.decomposition.non_negative_factorization", "sklearn.decomposition.NMF"),
+    ("sklearn.preprocessing.maxabs_scale", "sklearn.preprocessing.MaxAbsScaler"),


should robust_scale also be here?

glemaitre · 2023-06-22T16:06:46Z

Thanks @jeremiedbb.

…scikit-learn#26495)

…#26495)

…scikit-learn#26495)

skip nested val all functions

6af9c3c

jeremiedbb added No Changelog Needed Validation related to input validation labels Jun 2, 2023

jeremiedbb added this to the 1.3 milestone Jun 2, 2023

adrinjalali reviewed Jun 6, 2023

View reviewed changes

iter

88de34e

jeremiedbb commented Jun 7, 2023

View reviewed changes

adrinjalali reviewed Jun 9, 2023

View reviewed changes

jeremiedbb added 3 commits June 14, 2023 14:14

comment where skip_nested=False

3b96e5a

maxabs_scale is a class wrapper

6bd76c8

Merge remote-tracking branch 'upstream/main' into pr/jeremiedbb/26495

d561a25

adrinjalali reviewed Jun 14, 2023

View reviewed changes

glemaitre self-requested a review June 14, 2023 14:35

jeremiedbb mentioned this pull request Jun 22, 2023

MAINT parameter validation for metrics.pairwise.pairwise_kernels #26665

Merged

glemaitre approved these changes Jun 22, 2023

View reviewed changes

glemaitre merged commit 04575f5 into scikit-learn:main Jun 22, 2023

glemaitre mentioned this pull request Jun 22, 2023

FIX add missing prefer_skip_nested_validation in missing paired_distances #26671

Merged

jeremiedbb added the To backport PR merged in master that need a backport to a release branch defined based on the milestone. label Jun 27, 2023

jeremiedbb added a commit to jeremiedbb/scikit-learn that referenced this pull request Jun 29, 2023

MAINT Param validation: apply skip nested validation to all functions (…

4305eb1

…scikit-learn#26495)

jeremiedbb added a commit that referenced this pull request Jun 29, 2023

MAINT Param validation: apply skip nested validation to all functions (…

79f4ac9

…#26495)

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

MAINT Param validation: apply skip nested validation to all functions (…

f9bb7ed

…scikit-learn#26495)

		@@ -296,6 +296,7 @@ def test_function_param_validation(func_module):

		PARAM_VALIDATION_CLASS_WRAPPER_LIST = [

Uh oh!

MAINT Param validation: apply skip nested validation to all functions #26495

MAINT Param validation: apply skip nested validation to all functions #26495

Uh oh!

Conversation

jeremiedbb commented Jun 2, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

adrinjalali commented Jun 6, 2023

Uh oh!

jeremiedbb commented Jun 6, 2023

Uh oh!

adrinjalali left a comment

Choose a reason for hiding this comment

Uh oh!

adrinjalali Jun 6, 2023

Choose a reason for hiding this comment

Uh oh!

jeremiedbb left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jeremiedbb left a comment

Choose a reason for hiding this comment

Uh oh!

adrinjalali left a comment

Choose a reason for hiding this comment

Uh oh!

adrinjalali Jun 9, 2023

Choose a reason for hiding this comment

Uh oh!

adrinjalali Jun 9, 2023

Choose a reason for hiding this comment

Uh oh!

adrinjalali Jun 14, 2023

Choose a reason for hiding this comment

Uh oh!

glemaitre commented Jun 22, 2023

Uh oh!

Uh oh!

jeremiedbb commented Jun 2, 2023 •

edited

Loading