Skip to content

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.

jeremiedbb pushed a commit to jeremiedbb/scikit-learn that referenced this pull request Jul 15, 2025
@jeremiedbb jeremiedbb mentioned this pull request Jul 15, 2025
13 tasks
jeremiedbb pushed a commit to jeremiedbb/scikit-learn that referenced this pull request Jul 16, 2025
jeremiedbb pushed a commit to jeremiedbb/scikit-learn that referenced this pull request Jul 16, 2025
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