-
-
Notifications
You must be signed in to change notification settings - Fork 25.8k
API fix params validation in SGD inherited models #20683
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
API fix params validation in SGD inherited models #20683
Conversation
@adrinjalali would you be interested to have a look at this :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't really like to have a local variable named SGDEstimator
with upper case. I would rather use a explicit local variable name such as estimator_type
(or estimator_class
) or sgd_estimator_type
if want to be extra explicit.
Other than that, LGTM. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. A ton of the changes are not related to this PR though, they're just the rename of the variable, but I'm okay with that, it just made the review a bit harder.
Since this is a class name, does it not make sense to use the CapWords convention? |
It's not the name of any class. There is no scikit-learn class with |
+1 for reverting the change to the local variable name to keep the diff clean and focused on the main matter of PR. |
I reverting the changes. Now, it looks like a PR that @lorentzenchr will like :P |
O_o do we have an issue for this? is it related to this PR?
|
@adrinjalali nop this is linked with #20691 due to my autocomplete vs code import system that I just deactivated :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Co-authored-by: Thomas J. Fan <thomasjpfan@gmail.com>
Merging since we have a +2 and the comments have been addresed. |
+137 and -168 and make something in scikit-learn indeed scikit-learn API compliant (❓❗), yes 👍 . |
Welcome to historical reasons @lorentzenchr 😁 |
We should learn from history:smirk: |
partly addressing #12652
partly superseding #16945
Remove parameters validation from places other than
fit
andpartial_fit
to follow our API.I refactored the tests to use
pytest
parametrization. However, I did not remove any tests.I added the validation of the parameters for the
PassiveAggressive
estimators.The changes as mainly due to renamingklass
toSGDEstimator
for consistency.