[MRG] FIX bug in nested set_params usage #9999

jnothman · 2017-10-25T09:28:27Z

Issue where estimator is changed as well as its parameter: #9945 (comment)

Issue where estimator is changed as well as its parameter: scikit-learn#9945 (comment)

ogrisel

nitpicks but otherwise +1

ogrisel · 2017-10-25T09:38:52Z

sklearn/tests/test_base.py

+def test_set_params_updates_valid_params():
+    # Check that set_params tries to set SVC().C, not
+    # DecisionTreeClassifier().C
+    pipe = GridSearchCV(DecisionTreeClassifier(), {})


Why naming it pipe instead of something like gridsearchcv?

ogrisel · 2017-10-25T09:39:22Z

sklearn/tests/test_base.py

+    # Check that set_params tries to set SVC().C, not
+    # DecisionTreeClassifier().C
+    pipe = GridSearchCV(DecisionTreeClassifier(), {})
+    pipe.set_params(estimator=SVC(), estimator__C=1.0)


Could you please extend this test to do estimator__C=42.0 and then assert gridsearchcv.estimator.C == 42.0?

lesteve · 2017-10-25T09:41:20Z

sklearn/tests/test_base.py

+def test_set_params_updates_valid_params():
+    # Check that set_params tries to set SVC().C, not
+    # DecisionTreeClassifier().C
+    pipe = GridSearchCV(DecisionTreeClassifier(), {})


pipe may not be the best of names ;-)

jnothman · 2017-10-25T09:44:53Z

Sorry, I started with a Pipeline then realised it's not affected!

…

On 25 October 2017 at 20:41, Loïc Estève ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In sklearn/tests/test_base.py <#9999 (comment)> : > @@ -246,6 +246,13 @@ def set_params(self, **kwargs): estimator__min_samples_leaf=2) +def test_set_params_updates_valid_params(): + # Check that set_params tries to set SVC().C, not + # DecisionTreeClassifier().C + pipe = GridSearchCV(DecisionTreeClassifier(), {}) pipe may not be the best of names ;-) — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#9999 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAEz625DDrChwDCU8H3cTxegBIDTnRkOks5svwJCgaJpZM4QFsj-> .

jnothman · 2017-10-25T09:46:43Z

Thanks for the quick reviews!

lesteve · 2017-10-25T09:48:14Z

In the SO question, the OP mentions that this diff is working for him:

diff --git a/sklearn/base.py b/sklearn/base.py
index b653b7149..81c7e5dae 100644
--- a/sklearn/base.py
+++ b/sklearn/base.py
@@ -263,6 +263,7 @@ class BaseEstimator(object):
                 nested_params[key][sub_key] = value
             else:
                 setattr(self, key, value)
+                valid_params[key] = value
 
         for key, sub_params in nested_params.items():
             valid_params[key].set_params(**sub_params)

I checked indeed that the test passes with this patch which looked simpler than your current change. You know a lot more than me about this code, so there may be a reason why your current change is the way it is. Maybe there is a edge case that the simpler patch is not covering, if that is the case, it would be great to add a test for it.

jnothman · 2017-10-25T09:52:33Z

sorry. I didn't see that on my phone browser. yes it's a better patch. will fix. why didn't op make a pr??

…

On 25 Oct 2017 8:48 pm, "Loïc Estève" ***@***.***> wrote: In the SO question, the OP mentions that this diff is working for him: diff --git a/sklearn/base.py b/sklearn/base.py index b653b7149..81c7e5dae 100644 --- a/sklearn/base.py +++ b/sklearn/base.py @@ -263,6 +263,7 @@ class BaseEstimator(object): nested_params[key][sub_key] = value else: setattr(self, key, value) + valid_params[key] = value for key, sub_params in nested_params.items(): valid_params[key].set_params(**sub_params) I checked indeed that the test passes with this patch which looked simpler than your current change. You know a lot more than me about this code, so there may be a reason why your current change is the way it is. Maybe there is a edge case that the simpler patch is not covering, if that is the case, it would be great to add a test for it. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#9999 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAEz6ywoqnTFhgZV5px4Mlmwzyi84OoDks5svwPggaJpZM4QFsj-> .

marcus-voss · 2017-10-25T09:56:24Z

Hey, the OP of SO here. I could PR it, but as I am not very experienced with such open source work I was kinda intimidated by simply adding the PR. As @lesteve mentions, who knows what edge cases I may be breaking.

jnothman · 2017-10-25T09:59:28Z

I should probably have let you post a PR. But yes, contributors tend to be scared of this part of the codebase... Please do contribute in the future. Your SO post is the sort of stuff dreams are made of in the FOSS world; if only it were posted as a PR! I'd like to think that edge cases are tested cases.... :\

TomDLT · 2017-10-25T10:16:23Z

LGTM as well, thanks @marcus-voss for the clean fix !

marcus-voss · 2017-10-25T10:42:56Z

Hey @TomDLT and @jnothman, thanks for the kind words. For today just finding the bug and providing the fix indirectly for that great library already made my day. Next time, I'll definitely consider the PR!

ogrisel · 2017-10-25T13:23:51Z

Thanks all!

…cs/donigian-update-contribution-guidelines * 'master' of github.com:scikit-learn/scikit-learn: (23 commits) fixes scikit-learn#10031: fix attribute name and shape in documentation (scikit-learn#10033) [MRG+1] add changelog entry for fixed and merged PR scikit-learn#10005 issue scikit-learn#9633 (scikit-learn#10025) [MRG] Fix LogisticRegression see also should include LogisticRegressionCV(scikit-learn#9995) (scikit-learn#10022) [MRG + 1] Labels of clustering should start at 0 or -1 if noise (scikit-learn#10015) MAINT Remove redundancy in scikit-learn#9552 (scikit-learn#9573) [MRG+1] correct comparison in GaussianNB for 'priors' (scikit-learn#10005) [MRG + 1] ENH add check_inverse in FunctionTransformer (scikit-learn#9399) [MRG] FIX bug in nested set_params usage (scikit-learn#9999) [MRG+1] Fix LOF and Isolation benchmarks (scikit-learn#9798) [MRG + 1] Fix negative inputs checking in mean_squared_log_error (scikit-learn#9968) DOC Fix typo (scikit-learn#9996) DOC Fix typo: x axis -> y axis (scikit-learn#9985) improve example plot_forest_iris.py (scikit-learn#9989) [MRG+1] Deprecate pooling_func unused parameter in AgglomerativeClustering (scikit-learn#9875) DOC update news DOC Fix three typos in manifold documentation (scikit-learn#9990) DOC add missing dot in docstring DOC Add what's new for 0.19.1 (scikit-learn#9983) Improve readability of outlier detection example. (scikit-learn#9973) DOC: Fixed typo (scikit-learn#9977) ...

FIX bug in nested set_params usage

c890fd1

Issue where estimator is changed as well as its parameter: scikit-learn#9945 (comment)

jnothman added the Bug label Oct 25, 2017

jnothman changed the title ~~FIX bug in nested set_params usage~~ [MRG] FIX bug in nested set_params usage Oct 25, 2017

ogrisel approved these changes Oct 25, 2017

View reviewed changes

lesteve reviewed Oct 25, 2017

View reviewed changes

jnothman added 2 commits October 25, 2017 20:45

Tweak test

80e8792

Tweak test properly

1d2cf2d

Use simpler solution with credit to @marcus-voss

03c0a21

DOC what's new

0c6a7d9

ogrisel merged commit 102620f into scikit-learn:master Oct 25, 2017

maskani-moh pushed a commit to maskani-moh/scikit-learn that referenced this pull request Nov 15, 2017

[MRG] FIX bug in nested set_params usage (scikit-learn#9999)

abf1b17

jwjohnson314 pushed a commit to jwjohnson314/scikit-learn that referenced this pull request Dec 18, 2017

[MRG] FIX bug in nested set_params usage (scikit-learn#9999)

a2eabd0

This was referenced Jan 23, 2018

BaseEstimator.set_params(**params) is not truly recursive #10524

Closed

set_params fn for pipeline to maintain order of parameters #10439

Closed

rasbt mentioned this pull request May 31, 2018

[MRG] FEA: Stacking estimator for classification and regression #11047

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[MRG] FIX bug in nested set_params usage #9999

[MRG] FIX bug in nested set_params usage #9999

jnothman commented Oct 25, 2017

ogrisel left a comment

ogrisel Oct 25, 2017

ogrisel Oct 25, 2017

lesteve Oct 25, 2017

jnothman commented Oct 25, 2017 via email

jnothman commented Oct 25, 2017 via email

lesteve commented Oct 25, 2017 •

edited

Loading

jnothman commented Oct 25, 2017 via email

marcus-voss commented Oct 25, 2017

jnothman commented Oct 25, 2017 via email

TomDLT commented Oct 25, 2017

marcus-voss commented Oct 25, 2017

ogrisel commented Oct 25, 2017

[MRG] FIX bug in nested set_params usage #9999

[MRG] FIX bug in nested set_params usage #9999

Conversation

jnothman commented Oct 25, 2017

ogrisel left a comment

Choose a reason for hiding this comment

ogrisel Oct 25, 2017

Choose a reason for hiding this comment

ogrisel Oct 25, 2017

Choose a reason for hiding this comment

lesteve Oct 25, 2017

Choose a reason for hiding this comment

jnothman commented Oct 25, 2017 via email

jnothman commented Oct 25, 2017 via email

lesteve commented Oct 25, 2017 • edited Loading

jnothman commented Oct 25, 2017 via email

marcus-voss commented Oct 25, 2017

jnothman commented Oct 25, 2017 via email

TomDLT commented Oct 25, 2017

marcus-voss commented Oct 25, 2017

ogrisel commented Oct 25, 2017

lesteve commented Oct 25, 2017 •

edited

Loading