Surprise icon indicating copy to clipboard operation
Surprise copied to clipboard

Grid search classes should accept estimator instances instead of just classes

Open NicolasHug opened this issue 7 years ago • 2 comments

Scikit learn does, surprise doesn't, and that sucks :)

Current workaround for changing default parameter values that you want to keep constant across searches, e.g. with random_state:

grid = {'random_state': [one_unique_element_in_the_list], 'other_param': [a, b, c]...}

NicolasHug avatar Oct 10 '18 18:10 NicolasHug

I'd like to take a stab at this. One proposed solution:

  1. Ensure that all classes save arguments to the __init__ method as self.arg_name
  2. When generating grid searches create model objects which have the same arguments as some initial class overridden by the values for the parameter search (only if whats passed in is an instance of AlgoBase else just do normal behavior).

ZachGlassman avatar Nov 03 '18 17:11 ZachGlassman

Awesome! :)

For this I think we should try to follow as close as possible what scikit learn is doing (see e.g. their conventions for estimators). There's no need to reinvent the wheel here and your proposal is going in the right direction.

Ensure that all classes save arguments to the init method as self.arg_name

Yes! We should also ensure that none of those parameters/attributes are modified in __init__ or in any other method. Else, we might end up with different clones.

When generating grid searches create model objects which have the same arguments as some initial class overridden by the values for the parameter search

Yes, that's basically the clone method from scikit learn. This would require a get_params method I believe.

NicolasHug avatar Nov 03 '18 17:11 NicolasHug