Skip to content

MNT remove PA_C from SGD and (re-) use eta0 #31932

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

MNT remove PA_C from SGD and (re-) use eta0 #31932

wants to merge 4 commits into from

Conversation

lorentzenchr
Copy link
Member

Reference Issues/PRs

Follow-up of #29097.

What does this implement/fix? Explain your changes.

This PR removes the newly introduces (not yet released) PA_C parameter of SGD and reuses the already existing eta0 to specify the aggressiveness parameter for the passive-aggressive algorithm.

Any other comments?

No deprecation cycle need if merged before 1.8 release.

@lorentzenchr
MNT remove PA_C
ec32951 
* no deprecation cycle as PA_C was not yet released (as of scikit-learn 1.7)
@github-actions GitHub Actions
Copy link

github-actions bot commented Aug 12, 2025 •
edited
Loading

✔️ Linting Passed

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

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

@lorentzenchr
Copy link
Member Author

Question for reviewers: How can I add this PR to the whatsnew entry of #29097? The new towncrier is nice but I have no clue how to accomplish this (need it for other PRs too).

@adrinjalali @OmarManzoor friendly ping as you reviewed #29097.

@lorentzenchr lorentzenchr mentioned this pull request Aug 12, 2025
Open
@OmarManzoor
Copy link
Contributor

@lorentzenchr Maybe specify something like
the parameter PA_C which was added to SGD classes to accommodate the deprecation of PassiveAggressive classes has now been removed in favor of using the already existing eta0 parameter

But I think @adrinjalali might be able to provide a better suggestion in this regard

@lorentzenchr
Copy link
Member Author

@OmarManzoor Where should I use your suggestion?

@OmarManzoor
Copy link
Contributor

@OmarManzoor Where should I use your suggestion?

I was replying with respect the changelog entry question you mentioned

adrinjalali
adrinjalali reviewed Aug 19, 2025
View reviewed changes
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.

Otherwise LGTM.

doc/whats_new/upcoming_changes/sklearn.linear_model/29097.api.rst
Copy link
Member

Choose a reason for hiding this comment

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

I think the right way to do this is to have the exact same text for the changelog, just in a different file with the new PR number (am I correct @lesteve ?)

Copy link
Member Author

@lorentzenchr lorentzenchr Aug 19, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Before I change, I wait for @lesteve's answer.
The rendered docs of the whatsnew right now seems fine to me: On entry mentioning both PRs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@adrinjalali adrinjalali adrinjalali left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
cython module:linear_model
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@lorentzenchr @OmarManzoor @adrinjalali