Skip to content

FIX: Change limits of power_t param to [0, inf) #31474

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 8 commits into from
Jun 10, 2025

Conversation

ritvi-alagusankar
Copy link
Contributor

@ritvi-alagusankar ritvi-alagusankar commented Jun 3, 2025

Reference Issues/PRs

Fixes #22178. Also see #22215 comment for reference.

What does this implement/fix? Explain your changes.

  • Throw a warning when the power_t parameter is negative and warn the user that negative values will be invalid in two releases time.
  • Update the docstrings based on the changes to the constraints.

Copy link

github-actions bot commented Jun 3, 2025

✔️ Linting Passed

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

Generated for commit: 28086a6. Link to the linter CI: here

@betatim
Copy link
Member

betatim commented Jun 3, 2025

Thanks for creating this Pull Request (and getting involved in open-source!).

I think we should handle this via deprecation cycle. This means that code that today uses a negative value will get a warning that in two releases time this will become an invalid value. You can take a look at the section deprecating a parameter of the deprecation guide. We aren't doing exactly the same here, but I think that is the closest example to take inspiration from.

In addition to emitting a warning we need to update the docstrings to mention the deprecation and add a test to check that the warning is (not) raised when it should be.

@ritvi-alagusankar
Copy link
Contributor Author

Thanks for creating this Pull Request (and getting involved in open-source!).

I think we should handle this via deprecation cycle. This means that code that today uses a negative value will get a warning that in two releases time this will become an invalid value. You can take a look at the section deprecating a parameter of the deprecation guide. We aren't doing exactly the same here, but I think that is the closest example to take inspiration from.

In addition to emitting a warning we need to update the docstrings to mention the deprecation and add a test to check that the warning is (not) raised when it should be.

I have made the changes based on your suggestion. Please let me know if this is what you are looking for and if any further changes need to be made.

@ritvi-alagusankar ritvi-alagusankar requested a review from betatim June 3, 2025 15:31
Copy link
Member

@betatim betatim left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me modulo the docstring change and adding a changelog entry

@ritvi-alagusankar
Copy link
Contributor Author

@betatim I have changed the docstring and added the changelog entry, please let me know if this PR can be merged

@betatim betatim added Quick Review For PRs that are quick to review Waiting for Second Reviewer First reviewer is done, need a second one! labels Jun 4, 2025
Copy link
Member

@virchan virchan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Thanks for the PR, @ritvi-alagusankar!

@ritvi-alagusankar
Copy link
Contributor Author

@virchan gentle ping to help merge the PR

Copy link
Contributor

@OmarManzoor OmarManzoor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thanks @ritvi-alagusankar

@OmarManzoor OmarManzoor merged commit 1fae098 into scikit-learn:main Jun 10, 2025
36 checks passed
@reshamas
Copy link
Member

This is my first contribution, so please let me know if any changes are needed.

@ritvi-alagusankar
Congrats on your first contribution to scikit-learn!
And thank you for addressing this issue, which was opened more than 3 years ago.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
module:linear_model Quick Review For PRs that are quick to review Waiting for Second Reviewer First reviewer is done, need a second one!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

power_t: does it make sense for this parameter to have negative values
5 participants