Skip to content

Fixes API Deprecate n_alphas in LinearModelCV - #30616

Merged
adrinjalali merged 10 commits into
scikit-learn:mainfrom
KANNAHWORLD:LinearModelCV_api_refactor
Apr 23, 2025
Merged

Fixes API Deprecate n_alphas in LinearModelCV#30616
adrinjalali merged 10 commits into
scikit-learn:mainfrom
KANNAHWORLD:LinearModelCV_api_refactor

Conversation

@KANNAHWORLD

Copy link
Copy Markdown
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.

@github-actions

github-actions Bot commented Jan 9, 2025

Copy link
Copy Markdown

✔️ 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

@adrinjalali adrinjalali left a comment

Copy link
Copy Markdown
Member

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
Copy Markdown
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.

Comment thread sklearn/linear_model/_coordinate_descent.py Outdated
Comment thread sklearn/linear_model/_coordinate_descent.py Outdated
Comment thread sklearn/linear_model/_coordinate_descent.py Outdated
Comment thread sklearn/linear_model/_coordinate_descent.py Outdated
@jeremiedbb

Copy link
Copy Markdown
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.

@jeremiedbb jeremiedbb left a comment

Copy link
Copy Markdown
Member

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
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