Skip to content

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

Merged
merged 10 commits into from
Apr 23, 2025

Conversation

KANNAHWORLD
Copy link
Contributor

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 of n_alphas is preserved by passing an integer to alphas. 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 of alphas. 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.

Copy link

github-actions bot commented Jan 9, 2025

✔️ Linting Passed

All linting checks passed. Your pull request is in excellent shape! ☀️

Generated for commit: 4007700. Link to the linter CI: here

@KANNAHWORLD KANNAHWORLD marked this pull request as ready for review January 9, 2025 20:02
Copy link
Member

@adrinjalali adrinjalali 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.

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
Copy link
Member

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.

@jeremiedbb
Copy link
Member

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.

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

@adrinjalali adrinjalali changed the title Fixes API Deprecate n_alphas in LinearModelCV #30467 Fixes API Deprecate n_alphas in LinearModelCV Apr 23, 2025
@adrinjalali adrinjalali merged commit d8095e6 into scikit-learn:main Apr 23, 2025
37 checks passed
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.

API Deprecate n_alphas in LinearModelCV
3 participants