Skip to content
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

Merged
merged 3 commits into from Nov 3, 2021

Conversation

ArturoAmorQ
Copy link
Member

@ArturoAmorQ ArturoAmorQ commented Oct 29, 2021

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 a np.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 ensure alphas are converted during fit.
This PR also clarifies the docstring of _scale_alpha_inplace.

Any other comments?

Copy link
Member

@ogrisel ogrisel left a 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?

@glemaitre
Copy link
Member

Following @ogrisel comment, I think that we are missing a couple of changes that are removing np.asarray from _BaseRidgeCV. This would be fixing RidgeCV as part of #21406.

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 __init__ are equal to the IDs of the attribute of the instance after creating the instance. We will then detect a validation such as np.asarray in the __init__.

@ArturoAmorQ ArturoAmorQ marked this pull request as draft November 2, 2021 09:19
Copy link
Member

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

@ArturoAmorQ ArturoAmorQ changed the title TST Adapt _scale_alpha_inplace to ensure broadcasting MNT Postpone conversion of RidgeCV's alphas out of __init__ Nov 3, 2021
@ArturoAmorQ ArturoAmorQ marked this pull request as ready for review November 3, 2021 16:11
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.

LGTM.

@glemaitre glemaitre merged commit 2cf3c55 into scikit-learn:main Nov 3, 2021
glemaitre pushed a commit to glemaitre/scikit-learn that referenced this pull request Nov 29, 2021
samronsin pushed a commit to samronsin/scikit-learn that referenced this pull request Nov 30, 2021
@ArturoAmorQ ArturoAmorQ deleted the mnt_scale_alpha_inplace branch March 30, 2022 15:46
mathijs02 pushed a commit to mathijs02/scikit-learn that referenced this pull request Dec 27, 2022
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.

None yet

4 participants