MNT Param validation: Make it possible to mark a constraint as hidden #23558
Conversation
sklearn/linear_model/_base.py
Outdated
| _parameter_constraints = { | ||
| "fit_intercept": [bool], | ||
| "normalize": [StrOptions({"deprecated"}, internal={"deprecated"}), bool], | ||
| "normalize": [Internal(StrOptions({"deprecated"})), bool], |
There was a problem hiding this comment.
For developer ergonomics, Internal looks very similar to Interval. (At a glance, I thought it was Interval). What do you think of renaming Interval to Range?
There was a problem hiding this comment.
I kind of prefer Interval, and range being a python keyword I think it could also bring confusion. Maybe we can change Internal instead. What do you think about Private or Hidden ?
glemaitre
left a comment
glemaitre
left a comment
There was a problem hiding this comment.
I am fine with the rest of the PR.
glemaitre
changed the title
glemaitre
changed the title
| "n_init": [ | ||
| StrOptions({"auto", "warn"}, internal={"warn"}), | ||
| StrOptions({"auto"}), | ||
| Hidden(StrOptions({"warn"})), |
There was a problem hiding this comment.
The alternative is StrOptions({"warn"}, hidden=True), but then we would need _InstancesOf "public", to do InstancesOf(Criterion, hidden=True). Also all the other constraints will have to be "public" to be configured to hide.
With that in mind, I'm okay with the PR as is.
thomasjpfan
left a comment
thomasjpfan
left a comment
There was a problem hiding this comment.
LGTM
@glemaitre This PR changed a little bit since your approval. Do you want to take another look?
|
Still LGTM. Merging. |
3 participants
Some parameters accepts types as an unofficial way. For instance
criterionin trees can be an instance ofCriterionbut that's not documented and not meant to be documented.For these, we don't want the error message of the validation of this parameter to say that this unofficial type is valid. Hence this PR proposes to make it possible to mark a constraint as
internalsuch that it's not shown in the message.