Skip to content

MNT Param validation: Allow to skip validation of a parameter #23602

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 6 commits into from
Jun 22, 2022
Merged

MNT Param validation: Allow to skip validation of a parameter #23602

merged 6 commits into from
Jun 22, 2022

Conversation

jeremiedbb
Copy link
Member

For some parameters, we may want to skip validation, delegating it to the actual consumer of the param.

An example is the dtype parameter of OneHotEncoder, see #23579. Numpy dtype can be expressed in many different ways and we don't necessarily want to list them all here and keep it up to date. Instead we want to delegate validation to numpy since we don't deal with dtype but directly pass it to numpy/scipy functions.

I propose to allow specifying ["no validation"] as constraint for a parameter to skip validation for this parameter. It could also just be the string "no validation" (not in a list) to make it extra explicit that it's not even a constraint.
Alternatives can be ["any"] to express that anything is valid, or "delegate validation" maybe. I'm wide open to suggestions.

cc/ @thomasjpfan @glemaitre

@jeremiedbb jeremiedbb added No Changelog Needed Validation related to input validation labels Jun 13, 2022
@glemaitre
Copy link
Member

"no_validation" is already something that we use for the tag.
I like the idea of "delegate_validation" but it would be really defined for this case. "no_validation" would be broader.

I don't exactly know what would be the best.

@jjerphan
Copy link
Member

Thank you, @jeremiedbb.

To me, specifying the string "no validation" rather than a list containing seems more intuitive -- IMO specifying a list also would indicate that other values are passable for the validation which is not in that case.

@Diadochokinetic Diadochokinetic mentioned this pull request Jun 15, 2022
Closed
glemaitre
glemaitre approved these changes Jun 16, 2022
View reviewed changes
Copy link
Member

@glemaitre glemaitre left a comment

Choose a reason for hiding this comment

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

I think about it again and I think that as a first step this is fine to have "no validation". We can subcategories later on if we see that "no validation" is to wide (and introduce for instance "delegate", etc.).

thomasjpfan
thomasjpfan approved these changes Jun 21, 2022
View reviewed changes
Copy link
Member

@thomasjpfan thomasjpfan left a comment

Choose a reason for hiding this comment

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

I'm okay with adding a "no_validation" constraint.

Minor comment, otherwise, LGTM

sklearn/utils/_param_validation.py Outdated
@@ -44,6 +45,10 @@ def validate_parameter_constraints(parameter_constraints, params, caller_name):

for param_name, param_val in params.items():
constraints = parameter_constraints[param_name]

if constraints == ["no validation"]:
Copy link
Member

Choose a reason for hiding this comment

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

Minor nit: For the some consistency with _validate_data

Suggested change
if constraints == ["no validation"]:
if constraints == ["no_validation"]:

@jeremiedbb
Copy link
Member Author

jeremiedbb commented Jun 21, 2022 •
edited
Loading

To me, specifying the string "no validation" rather than a list containing seems more intuitive -- IMO specifying a list also would indicate that other values are passable for the validation which is not in that case.

I agree with @jjerphan's comment. ["no_validation"] makes one think that it's another constraint, while it's not and in particular it can't be associated with any other constraint. I also find a bare string (not in a list) better here. What do you think @glemaitre and @thomasjpfan ? (I checked and it's does not require any special change in the code)

@jeremiedbb
Copy link
Member Author

Changed to string "no_validation" not in a list after irl discussion with @glemaitre who agreed with my last comment.

@glemaitre glemaitre merged commit d7c3828 into scikit-learn:main Jun 22, 2022
@glemaitre
Copy link
Member

Thanks @jeremiedbb LGTM

@thomasjpfan thomasjpfan mentioned this pull request Jun 23, 2022
Merged
ogrisel pushed a commit to ogrisel/scikit-learn that referenced this pull request Jul 11, 2022
@jeremiedbb @ogrisel
MNT Param validation: Allow to skip validation of a parameter (scikit…
f5f954f 
…-learn#23602)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@thomasjpfan thomasjpfan thomasjpfan approved these changes

@glemaitre glemaitre glemaitre approved these changes

Assignees
No one assigned
Labels
module:utils No Changelog Needed Validation related to input validation
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@jeremiedbb @glemaitre @jjerphan @thomasjpfan