New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[MRG] FIX bug in nested set_params usage #9999

Merged
merged 5 commits into from Oct 25, 2017

Conversation

Projects
None yet
5 participants
@jnothman
Member

jnothman commented Oct 25, 2017

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

FIX bug in nested set_params usage
Issue where estimator is changed as well as its parameter: #9945 (comment)

@jnothman jnothman added the Bug label Oct 25, 2017

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

@ogrisel

nitpicks but otherwise +1

Show outdated Hide outdated sklearn/tests/test_base.py
Show outdated Hide outdated sklearn/tests/test_base.py
@jnothman

This comment has been minimized.

Show comment
Hide comment
@jnothman

jnothman Oct 25, 2017

Member
Member

jnothman commented Oct 25, 2017

jnothman added some commits Oct 25, 2017

@jnothman

This comment has been minimized.

Show comment
Hide comment
@jnothman

jnothman Oct 25, 2017

Member
Member

jnothman commented Oct 25, 2017

@lesteve

This comment has been minimized.

Show comment
Hide comment
@lesteve

lesteve Oct 25, 2017

Member

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.

Member

lesteve commented Oct 25, 2017

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

This comment has been minimized.

Show comment
Hide comment
@jnothman

jnothman Oct 25, 2017

Member
Member

jnothman commented Oct 25, 2017

@marcus-voss

This comment has been minimized.

Show comment
Hide comment
@marcus-voss

marcus-voss Oct 25, 2017

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.

marcus-voss commented Oct 25, 2017

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

This comment has been minimized.

Show comment
Hide comment
@jnothman

jnothman Oct 25, 2017

Member
Member

jnothman commented Oct 25, 2017

@TomDLT

This comment has been minimized.

Show comment
Hide comment
@TomDLT

TomDLT Oct 25, 2017

Member

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

Member

TomDLT commented Oct 25, 2017

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

@marcus-voss

This comment has been minimized.

Show comment
Hide comment
@marcus-voss

marcus-voss Oct 25, 2017

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!

marcus-voss commented Oct 25, 2017

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 ogrisel merged commit 102620f into scikit-learn:master Oct 25, 2017

6 checks passed

ci/circleci Your tests passed on CircleCI!
Details
codecov/patch Coverage not affected when comparing f84581b...0c6a7d9
Details
codecov/project 96.17% (+<.01%) compared to f84581b
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
lgtm analysis: Python No alert changes
Details
@ogrisel

This comment has been minimized.

Show comment
Hide comment
@ogrisel

ogrisel Oct 25, 2017

Member

Thanks all!

Member

ogrisel commented Oct 25, 2017

Thanks all!

donigian added a commit to donigian/scikit-learn that referenced this pull request Oct 28, 2017

Merge branch 'master' of github.com:scikit-learn/scikit-learn into do…
…cs/donigian-update-contribution-guidelines

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

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

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment