[MRG] Added FutureWarning in sgd models for tol parameter #12399
Conversation
if tol is None while max_iter is set
| warnings.warn( | ||
| "max_iter and tol parameters have been added in %s in " | ||
| "0.19. If max_iter is set but tol is left unset, the " | ||
| "default value for tol in 0.19 and 0.20 will be None " |
There was a problem hiding this comment.
I think the semantics of None here are unclear. Is None equivalent to 0?
| "default value for tol in 0.19 and 0.20 will be None " | ||
| "but will change in 0.21 to 1e-3. Specify tol to " | ||
| "silence this warning." % type(self).__name__, | ||
| ChangedBehaviorWarning) |
There was a problem hiding this comment.
I think this is technically meant to be a FutureWarning
There was a problem hiding this comment.
Right,ow it should be a FutureWarning since we didn't change yet.
|
all these warnings should be captured in the tests. You will see that tests will fail once you change it to |
|
(I wouldn't explicitly set tol) |
Not sure what you mean, where in the docs and about what? |
| @@ -31,6 +31,11 @@ | |||
| from sklearn.model_selection import RandomizedSearchCV | |||
|
|
|||
|
|
|||
| # 0.23. warning about tol not having its correct default value. | |||
| pytestmark = pytest.mark.filterwarnings( | |||
| "ignore:max_iter and tol parameters have been") | |||
There was a problem hiding this comment.
There was so many warnings in this file that I globally ignored them, for the whole module.
|
By a note in the docs I meant that the max_iter description should say that tol should be set at the same time for forwards compatibility |
I've done this for one of the modules that had tons of failing tests but not for the others, because it doesn't seem to be a common practice (in scikit-learn). But I find it better myself, yeah. Should I use the module-level ignore instead? |
But this is only true up to 0.21 right? |
|
Can you add mention of the changing default value to whatsnew in the change model section? |
|
though actually nevermind, I'll do that for 0.21, not here. |
…rn#12399) * Added ChangedBehaviorWarning in sgd models if tol is None while max_iter is set * Changed to FutureWarning and clarified None meaning * Ignored warningin tests * Ignore warnings in tests, round 2
…rn#12399) * Added ChangedBehaviorWarning in sgd models if tol is None while max_iter is set * Changed to FutureWarning and clarified None meaning * Ignored warningin tests * Ignore warnings in tests, round 2
…rn#12399) * Added ChangedBehaviorWarning in sgd models if tol is None while max_iter is set * Changed to FutureWarning and clarified None meaning * Ignored warningin tests * Ignore warnings in tests, round 2
…rn#12399) * Added ChangedBehaviorWarning in sgd models if tol is None while max_iter is set * Changed to FutureWarning and clarified None meaning * Ignored warningin tests * Ignore warnings in tests, round 2
…ikit-learn#12399)" This reverts commit e26d4d9.
…ikit-learn#12399)" This reverts commit e26d4d9.
…rn#12399) * Added ChangedBehaviorWarning in sgd models if tol is None while max_iter is set * Changed to FutureWarning and clarified None meaning * Ignored warningin tests * Ignore warnings in tests, round 2
Reference Issues/PRs
Addresses #12239 (comment)
What does this implement/fix? Explain your changes.
Added
FutureWarningin sgd models iftolisNonewhilemax_iteris set.Any other comments?