Skip to content

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

Merged
merged 12 commits into from
Jun 24, 2022
Merged

MAINT validate parameter in Ridge #23563

merged 12 commits into from
Jun 24, 2022

Conversation

Eschivo
Copy link
Contributor

@Eschivo Eschivo commented Jun 7, 2022

Reference Issues/PRs

Towards #23462

What does this implement/fix? Explain your changes.

Use of _validate_params for the Ridge family of algorithms

Copy link
Member

@jeremiedbb jeremiedbb left a 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.

@Eschivo
Copy link
Contributor Author

Eschivo commented Jun 10, 2022

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.

@jeremiedbb jeremiedbb added the Validation related to input validation label Jun 13, 2022
@Eschivo
Copy link
Contributor Author

Eschivo commented Jun 13, 2022

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.

@Eschivo Eschivo changed the title [WIP] Use of _validate_params in Ridge [MRG] Use of _validate_params in Ridge Jun 23, 2022
@jeremiedbb
Copy link
Member

@eliaschiavon I directly pushed some changes to take into account the recent improvements in the validation mechanism.

Copy link
Member

@jeremiedbb jeremiedbb left a comment

Choose a reason for hiding this comment

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

LGTM

@glemaitre glemaitre self-requested a review June 24, 2022 14:58
@glemaitre glemaitre changed the title [MRG] Use of _validate_params in Ridge MAINT validate parameter in Ridge Jun 24, 2022
@@ -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)):
Copy link
Member

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.

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.

Otherwise LGTM.

@glemaitre glemaitre merged commit b772113 into scikit-learn:main Jun 24, 2022
@Eschivo
Copy link
Contributor Author

Eschivo commented Jun 24, 2022

Thank you for your work @jeremiedbb and @glemaitre !

@Eschivo Eschivo deleted the ridge branch June 24, 2022 21:49
ogrisel pushed a commit to ogrisel/scikit-learn that referenced this pull request Jul 11, 2022
Co-authored-by: jeremie du boisberranger <jeremiedbb@yahoo.fr>
Co-authored-by: Jérémie du Boisberranger <34657725+jeremiedbb@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants