-
-
Notifications
You must be signed in to change notification settings - Fork 25.8k
MNT Postpone conversion of RidgeCV
's alphas
out of __init__
#21506
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
MNT Postpone conversion of RidgeCV
's alphas
out of __init__
#21506
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.
Could you please add a new test (or expand an existing test) to justify the need for this change?
Following @ogrisel comment, I think that we are missing a couple of changes that are removing However, the common test implemented for #21406 is not good enough to detect these cases. I assume that we would like to check that the IDs of the provided parameters in |
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.
As this PR encompasses the scope of #21406, its title can be rewritten to mention the postponed conversion of alphas
.
_scale_alpha_inplace
to ensure broadcastingRidgeCV
's alphas
out of __init__
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.
Reference Issues/PRs
Encompasses the scope of #21406. See also #19426.
What does this implement/fix? Explain your changes.
alphas
used to be converted to anp.ndarray
inside__init__
.This is now postponed to
fit
to follow the scikit-learn API.As
alphas
can now be a list, we modify the_scale_alpha_inplace
test accordingly.A new test
ridgecv_alphas_conversion
is created to ensurealphas
are converted duringfit
.This PR also clarifies the docstring of
_scale_alpha_inplace
.Any other comments?