-
-
Notifications
You must be signed in to change notification settings - Fork 25.8k
MAINT validate parameter in Ridge #23563
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.
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR @eliaschiavon. Here are some suggestions.
The next step would to call self._validate_params
in fit in Ridge
and RidgeClassifier
which inherit from this base class. Then check if previous validations are now obsolete and can be removed.
Thank you very much for the feedback, Jeremie. I'm gonna implement all the things the next week since I'm away during the weekend. |
…ter constraints _BaseRidge
-removed Ridge and RidgeClassifier from list of PARAM_VALIDATION_ESTIMATORS_TO_IGNORE
normalize in parameter constraints in BaseRidge is now an Hidden StrOptions
All the things should be ok for Ridge and RidgeClassifier. The only thing failing in the test is the implementation of the normalize parameter as Hidden, as you suggested in the comments above. |
@eliaschiavon I directly pushed some changes to take into account the recent improvements in the validation mechanism. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
sklearn/linear_model/_ridge.py
Outdated
@@ -637,7 +641,7 @@ def _ridge_regression( | |||
|
|||
# Some callers of this method might pass alpha as single | |||
# element array which already has been validated. | |||
if alpha is not None and not isinstance(alpha, (np.ndarray, tuple)): | |||
if alpha is not None and not isinstance(alpha, (np.ndarray)): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we can probably remove the paranthesis around np.ndarray
.
We are sure that we don't have any case where the value in the tuple
have been validated but not transformed to a NumPy array.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Otherwise LGTM.
Thank you for your work @jeremiedbb and @glemaitre ! |
Co-authored-by: jeremie du boisberranger <jeremiedbb@yahoo.fr> Co-authored-by: Jérémie du Boisberranger <34657725+jeremiedbb@users.noreply.github.com>
Reference Issues/PRs
Towards #23462
What does this implement/fix? Explain your changes.
Use of _validate_params for the Ridge family of algorithms