[MRG+1] fix for erroneous max_iter and tol warnings for SGDClassifier when using partial_fit #10053
Conversation
|
You have flake8 errors |
|
flake8 issues have been fixed |
| @@ -538,7 +539,7 @@ def partial_fit(self, X, y, classes=None, sample_weight=None): | |||
| ------- | |||
| self : returns an instance of self. | |||
| """ | |||
| self._validate_params() | |||
| self._validate_params(for_partial_fit=True) | |||
There was a problem hiding this comment.
I think using set_max_iter=False should suffice
There was a problem hiding this comment.
Ok, I will make that change.
There was a problem hiding this comment.
This hasn't been addressed and it seems cleaner not to add another parameter if it's possible.
There was a problem hiding this comment.
It was attempted, but was not trivial. We could have a different value for the parameter, but I figure it's hardly worth the bother for a deprecation.
There was a problem hiding this comment.
It was attempted, but was not trivial. We could have a different value for the parameter, but I figure it's hardly worth the bother for a deprecation.
| @@ -1211,6 +1211,9 @@ def init(max_iter=None, tol=None, n_iter=None): | |||
| assert_no_warnings(init, None, 1e-3, None) | |||
| assert_no_warnings(init, 100, 1e-3, None) | |||
|
|
|||
| # Test that for_partial_fit will not throw warnings for max_iter or tol | |||
| assert_no_warnings(init, None, None, None, True) | |||
|
|
|||
There was a problem hiding this comment.
Why not test if partial_fit itself raises a warning. Testing the helper is a bit strange, even if it seems that's already what we're doing...
|
@jnothman the |
e255cb7 to
9b624ea
Compare
|
What other issues? |
|
https://travis-ci.org/scikit-learn/scikit-learn/jobs/296053432 This is the result of just using the The issue seems to be that |
|
okay. well then I'd rather set_max_iter='quiet' than a separate parameter,
but this is okay.
|
|
If there is a sufficiently compelling reason to stick to using one parameter for both these cases, I'm happy to do that but I think such a parameter would need to be renamed from |
|
No, there's no compelling reason... these are ephemeral in that the code
will be removed in a year's time or so. I just would like it to be as
readable as possible in the meantime.
|
|
Do I need to add [MRG] to the title for this to be merged? |
|
@jnothman anything else you need to get this merged? |
|
@amueller Figured I'd ping you, since you made the initial issue |
|
lgtm. |
|
Thanks for contributing, @siftikha! |
|
Hm this undid #10050 :-/ |
… when using partial_fit (scikit-learn#10053) * partial fit warnings disabled * partial fit warnings disabled for regressor * style improved * tests added * pycodestyle passing * rejiggered format * fixed style issues
Fixes #10051
Added an optional argument
for_partial_fit=Falseto_validate_paramswhich bypasses warnings aboutmax_iterandtoladded
for_partial_fit=Trueto_validate_paramscalls inpartial_fitmethods for bothsgdclassiferandsgdregressorI appreciate that this could have been done through the use of
set_max_iter=Falsebut it seemed clearer to me to have a dedicated flag.first contribution to this project, so I apologize if I've done something horrifically wrong