FIX changed_only=True with kwargs parameters (#17205)

NicolasHug · adrinjalali · commit 1d40392a64c8 · 2020-05-19T10:00:50.000+02:00
diff --git a/doc/whats_new/v0.23.rst b/doc/whats_new/v0.23.rst
@@ -16,9 +16,16 @@ Changelog
 ......................
 
 - |Fix| Fixed a bug in :class:`cluster.KMeans` where the sample weights
-  provided by the user was modified in place. :pr:`17204` by
+  provided by the user were modified in place. :pr:`17204` by
   :user:`Jeremie du Boisberranger <jeremiedbb>`.
 
+Miscellaneous
+.............
+
+- |Fix| Fixed a bug in the `repr` of third-party estimators that use a
+  `**kwargs` parameter in their constructor, when `changed_only` is True
+  which is now the default. :pr:`17205` by `Nicolas Hug`_.
+
 .. _changes_0_23:
 
 Version 0.23.0
diff --git a/sklearn/utils/_pprint.py b/sklearn/utils/_pprint.py
@@ -94,9 +94,11 @@ def _changed_params(estimator):
                         estimator.__init__)
     init_params = signature(init_func).parameters
     init_params = {name: param.default for name, param in init_params.items()}
+
     for k, v in params.items():
-        if (repr(v) != repr(init_params[k]) and
-                not (is_scalar_nan(init_params[k]) and is_scalar_nan(v))):
+        if (k not in init_params or (  # happens if k is part of a **kwargs
+                repr(v) != repr(init_params[k]) and
+                not (is_scalar_nan(init_params[k]) and is_scalar_nan(v)))):
             filtered_params[k] = v
     return filtered_params
 
diff --git a/sklearn/utils/tests/test_pprint.py b/sklearn/utils/tests/test_pprint.py
@@ -8,7 +8,7 @@
 from sklearn.pipeline import make_pipeline
 from sklearn.base import BaseEstimator, TransformerMixin
 from sklearn.feature_selection import SelectKBest, chi2
-from sklearn import set_config
+from sklearn import set_config, config_context
 
 
 # Ignore flake8 (lots of line too long issues)
@@ -538,3 +538,39 @@ def test_builtin_prettyprinter():
     # Used to be a bug
 
     PrettyPrinter().pprint(LogisticRegression())
+
+
+def test_kwargs_in_init():
+    # Make sure the changed_only=True mode is OK when an argument is passed as
+    # kwargs.
+    # Non-regression test for
+    # https://github.com/scikit-learn/scikit-learn/issues/17206
+
+    class WithKWargs(BaseEstimator):
+        # Estimator with a kwargs argument. These need to hack around
+        # set_params and get_params. Here we mimic what LightGBM does.
+        def __init__(self, a='willchange', b='unchanged', **kwargs):
+            self.a = a
+            self.b = b
+            self._other_params = {}
+            self.set_params(**kwargs)
+
+        def get_params(self, deep=True):
+            params = super().get_params(deep=deep)
+            params.update(self._other_params)
+            return params
+
+        def set_params(self, **params):
+            for key, value in params.items():
+                setattr(self, key, value)
+                self._other_params[key] = value
+            return self
+
+    est = WithKWargs(a='something', c='abcd', d=None)
+
+    expected = "WithKWargs(a='something', c='abcd', d=None)"
+    assert expected == est.__repr__()
+
+    with config_context(print_changed_only=False):
+        expected = "WithKWargs(a='something', b='unchanged', c='abcd', d=None)"
+        assert expected == est.__repr__()
