[MRG+1] Accept keyword parameters to hyperparameter search fit methods by stephen-hoover · Pull Request #8278 · scikit-learn/scikit-learn

stephen-hoover · 2017-02-03T00:30:38Z

Reference Issue

What does this implement/fix? Explain your changes.

This PR allows the hyperparameter search classes in sklearn.model_selection._search (i.e. GridSearchCV and RandomizedSearchCV) to accept keyword parameters and pass them to the wrapped Estimator. This brings their fit methods into closer compliance with the Estimator API. This PR does not address issues #8158 or #4632 which deal with passing parameters to the scoring function.

To ease testing of the new parameter, I added an extra argument to the sklearn.utils.mocking.CheckingClassifier which allows it to enforce the presence of keyword fit arguments and verify that they have the expected length.

Any other comments?

The ultimate fate of fit parameters on these methods will depend on the resolution to #4497, but I think the change in this PR conforms to present usage without establishing any new conventions. Accepting keyword parameters in these fit methods is something that I (and I think other users) would expect.

One limitation of this PR is that the groups parameter is still reserved by the fit method and not passed to the wrapped Estimator. Perhaps a solution to that would wait on #4497, but I think it doesn't need to be solved in this PR.

I also had to choose what to do if users try to provide fit parameters through both the constructor and the fit method. I chose to emit a warning and ignore parameters passed through the constructor (since that behavior is deprecated), but I don't feel strongly about that decision.

I searched through existing documentation and tests, but I was unable to find any uses of the fit_params constructor argument for either GridSearchCV or RandomizedSearchCV.

Deprecate ``fit_params`` as a constructor argument to the hyperparameter search classes and instead accept keyword parameters to the ``fit`` methods. This makes the ``fit`` methods of these functions conform to the Estimator API and allows the use of hyperparameter search functions in other CV utility functions such as ``cross_val_predict``.

stephen-hoover · 2017-02-03T14:19:08Z

@jnothman , this is my minimal fix to #2879 .

jnothman · 2017-02-04T20:43:56Z

Thanks Stephen. I'm low on time at the moment, and will work through my backlog eventually!

…

On 4 Feb 2017 1:19 am, "Stephen Hoover" ***@***.***> wrote: @jnothman <https://github.com/jnothman> , this is my minimal fix to #2879 <#2879> . — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#8278 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAEz6yG7gsd6r1ZiBwl8VixrUInjN7Ajks5rYzdegaJpZM4L10eQ> .

ogrisel · 2017-02-08T08:33:58Z

sklearn/utils/mocking.py

    """
    def __init__(self, check_y=None,
-                 check_X=None, foo_param=0):
+                 check_X=None, foo_param=0,


cosmetic: put more args on the first line as long as they fit under 80 columns

ogrisel · 2017-02-08T08:36:27Z

sklearn/utils/mocking.py

        self.classes_ = np.unique(check_array(y, ensure_2d=False,
                                              allow_nd=True))
+        if self.expected_fit_params:
+            missing = (set(np.atleast_1d(self.expected_fit_params)) -


Why converting to an numpy array with np.atleast_1d before converting to a set? Converting the list to a set is enough.

The np.atleast_1d call converts a string to a one-element array, so that users could give a string input to expected_fit_params instead of a one-element list.

In this case, without the array casting there would be an obvious and loud error if someone forgot to make that input a list, and there's not much benefit to saving two characters for a test helper. I'll remove the np.atleast_1d.

ogrisel · 2017-02-08T08:40:53Z

Please add a new test to check that the deprecation warning is raised as expected.

ogrisel

Please also remove the fit_params parameter from the class docstrings and do a review of the full code base with git grep fit_params= to check that fit_params are no longer passed as input to the parameter search class constructors and update the code to pass those to the fit method instead.

ogrisel · 2017-02-08T08:39:14Z

sklearn/model_selection/tests/test_search.py

+    grid_search = GridSearchCV(clf, {'foo_param': [1, 2, 3]})
+
+    # The CheckingClassifer generates an assertion error if
+    # a parameter is missing or has length != len(X).


Instead of putting a comment to explain what CheckingClassifier should raise in that case, please add an assertion with assert_raises.

Consider assert_raises_regex to check the error message on top of checking the exception type.

ogrisel · 2017-02-08T08:39:45Z

sklearn/model_selection/tests/test_search.py

+    random_search = RandomizedSearchCV(clf, {'foo_param': [0]}, n_iter=1)
+
+    # The CheckingClassifer generates an assertion error if
+    # a parameter is missing or has length != len(X).


Please also add an assertion with assert_raises here.

ogrisel · 2017-02-08T08:45:16Z

LGTM besides the requested changes.

codecov · 2017-02-08T20:43:50Z

Codecov Report

❗ No coverage uploaded for pull request base (master@53f8082). Click here to learn what that means.

@@            Coverage Diff            @@
##             master    #8278   +/-   ##
=========================================
  Coverage          ?   94.74%           
=========================================
  Files             ?      342           
  Lines             ?    60711           
  Branches          ?        0           
=========================================
  Hits              ?    57519           
  Misses            ?     3192           
  Partials          ?        0

Impacted Files	Coverage Δ
sklearn/linear_model/tests/test_ridge.py	`100% <100%> (ø)`
sklearn/model_selection/tests/test_search.py	`99.28% <100%> (ø)`
sklearn/utils/mocking.py	`100% <100%> (ø)`
sklearn/linear_model/ridge.py	`93.88% <100%> (ø)`
sklearn/model_selection/_search.py	`97.85% <100%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 53f8082...9604c1d. Read the comment docs.

stephen-hoover · 2017-02-08T20:50:02Z

@ogrisel , I've made the changes that you and @lesteve requested.

ogrisel

Thank you very much, LGTM.

GaelVaroquaux · 2017-02-09T06:42:17Z

I am +1 to merge. This will be modified by the sample_props, but is it indeed going in the right direction in terms of API.

scikit-learn#8278) * ENH Accept keyword parameters to hyperparameter search fit methods Deprecate ``fit_params`` as a constructor argument to the hyperparameter search classes and instead accept keyword parameters to the ``fit`` methods. This makes the ``fit`` methods of these functions conform to the Estimator API and allows the use of hyperparameter search functions in other CV utility functions such as ``cross_val_predict``. * CR: Expanded tests, remove deprecated use in Ridge * Make tests consistent in Python 2 and 3

ecampana · 2017-03-11T07:34:10Z

Does GridSearchCV allow using a Pipeline estimator with fit_params, e.g. fit_params={'sample_weight': sample_weight}, such that sample_weight is passed from the grid.fit() method to the pipe.fit() method which is then ultimately passed to a classifier fit() method? I posted this question to #2630 as well. I am involved in the HEP community, and we are very interested in finding out if this type of procedure is at all possible as our final goal is to re-implement our analysis purely with scikit-learn. Thank you for your time.

stephen-hoover · 2017-03-11T12:11:40Z

@ecampana , yes, Pipelines accept fit parameters and pass them to their components. I commented on your question in #2630 , since you had examples there. But use fit parameters in fit, rather than in the GridSearchCV's constructor. You do still need to specify which step of the pipeline should get the fit parameter, so you can't use sample_weight= by itself. I see in the examples you gave that you do have the right parameters names to give to the Pipeline, though.

ecampana · 2017-03-16T00:36:15Z

@stephen-hoover, thank you for your reply and confirming that GridSearchCV and Pipeline in conjunction accomplish what I was hoping for but for which I was not quite uncertain. I have made further comments to my question in #2630 if you would by any chance have the time for it. Thanks again!

scikit-learn#8278) * ENH Accept keyword parameters to hyperparameter search fit methods Deprecate ``fit_params`` as a constructor argument to the hyperparameter search classes and instead accept keyword parameters to the ``fit`` methods. This makes the ``fit`` methods of these functions conform to the Estimator API and allows the use of hyperparameter search functions in other CV utility functions such as ``cross_val_predict``. * CR: Expanded tests, remove deprecated use in Ridge * Make tests consistent in Python 2 and 3

ogrisel reviewed Feb 8, 2017

View reviewed changes

ogrisel requested changes Feb 8, 2017

View reviewed changes

Stephen Hoover added 2 commits February 8, 2017 13:51

CR: Expanded tests, remove deprecated use in Ridge

effea9c

Make tests consistent in Python 2 and 3

9604c1d

ogrisel approved these changes Feb 8, 2017

View reviewed changes

ogrisel changed the title ~~[MRG] Accept keyword parameters to hyperparameter search fit methods~~ [MRG+1] Accept keyword parameters to hyperparameter search fit methods Feb 8, 2017

ogrisel added the Waiting for Reviewer label Feb 8, 2017

GaelVaroquaux merged commit 3a0ea19 into scikit-learn:master Feb 9, 2017

jnothman mentioned this pull request Feb 9, 2017

[API] Consistent API for attaching properties to samples #4497

Closed

stephen-hoover mentioned this pull request Mar 11, 2017

Make Pipeline compatible with AdaBoost #2630

Closed

Przemo10 mentioned this pull request Mar 17, 2017

update fork (#1) #8606

Closed

glemaitre mentioned this pull request May 19, 2017

DecisionTreeRegressor and DecisionTreeClassifier document lack the explanation of parameter min_impurity_split #8903

Closed

Uh oh!

Conversation

stephen-hoover commented Feb 3, 2017

Reference Issue

What does this implement/fix? Explain your changes.

Any other comments?

Uh oh!

stephen-hoover commented Feb 3, 2017

Uh oh!

jnothman commented Feb 4, 2017 via email

Uh oh!

ogrisel Feb 8, 2017

Choose a reason for hiding this comment

Uh oh!

ogrisel Feb 8, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stephen-hoover Feb 8, 2017

Choose a reason for hiding this comment

Uh oh!

ogrisel commented Feb 8, 2017

Uh oh!

ogrisel left a comment

Choose a reason for hiding this comment

Uh oh!

ogrisel Feb 8, 2017

Choose a reason for hiding this comment

Uh oh!

lesteve Feb 8, 2017

Choose a reason for hiding this comment

Uh oh!

ogrisel Feb 8, 2017

Choose a reason for hiding this comment

Uh oh!

ogrisel commented Feb 8, 2017

Uh oh!

codecov bot commented Feb 8, 2017

Codecov Report

Uh oh!

stephen-hoover commented Feb 8, 2017

Uh oh!

ogrisel left a comment

Choose a reason for hiding this comment

Uh oh!

GaelVaroquaux commented Feb 9, 2017

Uh oh!

ecampana commented Mar 11, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

stephen-hoover commented Mar 11, 2017

Uh oh!

ecampana commented Mar 16, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

ogrisel Feb 8, 2017 •

edited

Loading

ecampana commented Mar 11, 2017 •

edited

Loading

ecampana commented Mar 16, 2017 •

edited

Loading