Repository: scikit-learn/scikit-learn
Pull request: [MRG+1]: avoid FutureWarning in BaseSGD.set_params
#### Reference Issue
Fixes #9752


#### What does this implement/fix? Explain your changes.
Adds a set_future_warning flag in BaseSGD to help avoid FutureWarnings
in uses of __init__ or set_params.

Problem statement:
BaseSGD should not warn quite so often
Because of some strange legacy validation in __init__ (and set_params) as well as in fit and partial_fit (probably excessive in any case), the FutureWarning in BaseSGD is repeated when warn='always' is used. The FutureWarning should only be issued in uses of `fit` and `partial_fit`, and not during `__init__` or `set_params`.

Hints from issue discussion:
Would removing self._validate_params() from __init__ and set_params solve this issue?
well it would, but with side effects that would result in tests failing

On 13 Sep 2017 10:35 pm, "Vrishank Bhardwaj" <notifications@github.com>
wrote:

> Would removing self._validate_params() from *init* and set_params solve
> this issue?
>
> —
> You are receiving this because you authored the thread.
> Reply to this email directly, view it on GitHub
> <https://github.com/scikit-learn/scikit-learn/issues/9752#issuecomment-329153666>,
> or mute the thread
> <https://github.com/notifications/unsubscribe-auth/AAEz6yGpQZfc33ET_Pzp5-LVgy4NmYocks5sh8wogaJpZM4PV-iH>
> .
>

How should I go about tackling this issue?

Here's a snippet from the file 
`        # current tests expect init to do parameter validation
        # but we are not allowed to set attributes`
Maybe you could add a flag parameter to the validation method.

On 13 September 2017 at 23:14, Vrishank Bhardwaj <notifications@github.com>
wrote:

> How should I go about tackling this issue?
>
> Here's a snippet from the file
> # current tests expect init to do parameter validation # but we are not
> allowed to set attributes
>
> —
> You are receiving this because you authored the thread.
> Reply to this email directly, view it on GitHub
> <https://github.com/scikit-learn/scikit-learn/issues/9752#issuecomment-329163734>,
> or mute the thread
> <https://github.com/notifications/unsubscribe-auth/AAEz62_jC0XmK3IetL6d8prrYyTgTUNUks5sh9UvgaJpZM4PV-iH>
> .
>

I've added a set_future_warning flag. How do I test my changes locally?
If it's about building the documentation, you can use "make doc" if you
have appropriate dependencies installed.

On 13 September 2017 at 23:34, Vrishank Bhardwaj <notifications@github.com>
wrote:

> I've added a set_future_warning flag. How do I test my changes locally?
>
> —
> You are receiving this because you authored the thread.
> Reply to this email directly, view it on GitHub
> <https://github.com/scikit-learn/scikit-learn/issues/9752#issuecomment-329169326>,
> or mute the thread
> <https://github.com/notifications/unsubscribe-auth/AAEz6_hJzbSvk8TRsfUW_OUhQ3yWslRQks5sh9nbgaJpZM4PV-iH>
> .
>

Commit message: Update stochastic_gradient.py

Patch to review:
diff --git a/sklearn/linear_model/stochastic_gradient.py b/sklearn/linear_model/stochastic_gradient.py
index 4a6e6831edf44..18021a7a1ed84 100644
--- a/sklearn/linear_model/stochastic_gradient.py
+++ b/sklearn/linear_model/stochastic_gradient.py
@@ -50,7 +50,7 @@ def __init__(self, loss, penalty='l2', alpha=0.0001, C=1.0,
                  l1_ratio=0.15, fit_intercept=True, max_iter=None, tol=None,
                  shuffle=True, verbose=0, epsilon=0.1, random_state=None,
                  learning_rate="optimal", eta0=0.0, power_t=0.5,
-                 warm_start=False, average=False, n_iter=None):
+                 warm_start=False, average=False, n_iter=None, set_future_warning=True):
         self.loss = loss
         self.penalty = penalty
         self.learning_rate = learning_rate
@@ -69,13 +69,14 @@ def __init__(self, loss, penalty='l2', alpha=0.0001, C=1.0,
         self.n_iter = n_iter
         self.max_iter = max_iter
         self.tol = tol
+        self.set_future_warning = set_future_warning
         # current tests expect init to do parameter validation
         # but we are not allowed to set attributes
-        self._validate_params(set_max_iter=False)
+        self._validate_params(set_max_iter=False, set_future_warning=False)
 
     def set_params(self, *args, **kwargs):
         super(BaseSGD, self).set_params(*args, **kwargs)
-        self._validate_params()
+        self._validate_params(set_future_warning=False)
         return self
 
     @abstractmethod
@@ -120,14 +121,15 @@ def _validate_params(self, set_max_iter=True):
             self._tol = None
 
         elif self.tol is None and self.max_iter is None:
-            warnings.warn(
-                "max_iter and tol parameters have been added in %s in 0.19. If"
-                " both are left unset, they default to max_iter=5 and tol=None"
-                ". If tol is not None, max_iter defaults to max_iter=1000. "
-                "From 0.21, default max_iter will be 1000, "
-                "and default tol will be 1e-3." % type(self), FutureWarning)
-            # Before 0.19, default was n_iter=5
-            max_iter = 5
+            if self.set_future_warning:
+                warnings.warn(
+                    "max_iter and tol parameters have been added in %s in 0.19. If"
+                    " both are left unset, they default to max_iter=5 and tol=None"
+                    ". If tol is not None, max_iter defaults to max_iter=1000. "
+                    "From 0.21, default max_iter will be 1000, "
+                    "and default tol will be 1e-3." % type(self), FutureWarning)
+                # Before 0.19, default was n_iter=5
+                max_iter = 5
         else:
             max_iter = self.max_iter if self.max_iter is not None else 1000
         self._max_iter = max_iter


Code review comments:
diff_hunk: @@ -50,7 +50,7 @@ def __init__(self, loss, penalty='l2', alpha=0.0001, C=1.0,
                  l1_ratio=0.15, fit_intercept=True, max_iter=None, tol=None,
                  shuffle=True, verbose=0, epsilon=0.1, random_state=None,
                  learning_rate="optimal", eta0=0.0, power_t=0.5,
-                 warm_start=False, average=False, n_iter=None):
+                 warm_start=False, average=False, n_iter=None, set_future_warning=True):
path: sklearn/linear_model/stochastic_gradient.py
text: @user1:
I don't think we want this as a constructor parameter. It should be a parameter to `_validate_params`

@author:
Thanks. I'll modify the code.