-
-
Notifications
You must be signed in to change notification settings - Fork 25.8k
Fixes API Deprecate n_alphas in LinearModelCV #30616
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
Fixes API Deprecate n_alphas in LinearModelCV #30616
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.
I left a few comments, please fix the rest of the PR accordingly.
Also, the tests need to be fixed so that they don't raise a warning, and we need explicit tests for the warning and the value error.
@@ -0,0 +1,13 @@ | |||
```rst |
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.
Please have a look at the other changelog entries.
You don't need the triple backtick and the rst
tag here, and the lines need to be indented after the first line. Also, you need to rearrange the changelog into a single bullet point and convey the message in that single point.
I synchronized with main and addressed the review comments. I made a slight modification because imo we don't want to raise a warning when both params are left to their default value, even though default alphas is "warn". In that case the future default behavior will be 100 which corresponds to the current default behavior. I don't want that users leaving both params to their default value get a warning even though the behavior won't change. I updated the code and tests accordingly. |
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
Fixes #30467
Updated LinearModelCV and derived classes LassoCV, ElasticNetCV, MultiTaskElasticNetCV, MultiTaskLassoCV to remove
n_alphas
parameter from the constructor.alphas
parameter is updated to support an integer or array-like argument. Functionality ofn_alphas
is preserved by passing an integer toalphas
. Parameter_constraints updated accordingly.Unit tests created to verify correct warning messages are raised upon usage of
n_alphas
parameter. Unit tests created to verify correct warning message if using the default value ofalphas
. All unit tests passing and all warnings suppressed on existing test cases using filter warnings.All classes are backwards compatible with
n_alphas
until 1.8, before it will be removed. doc_strings updated to effectively communicate changes.