Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Do not do grid search if no param grids are provided #378

Merged
merged 5 commits into from
Oct 30, 2017
Merged

Do not do grid search if no param grids are provided #378

merged 5 commits into from
Oct 30, 2017

Conversation

desilinguist
Copy link
Member

@desilinguist desilinguist commented Oct 30, 2017 •
edited
Loading

  • We might have a situation where the default parameter grid for a learner is empty (this is the case for some new regressors that have been added as part of PR Add new learners (mostly regressors) #377). In that case, if the user also doesn't provide a parameter grid to search but grid_search is True, there's no point actually doing the grid search and we should just use the default hyperparameters.
  • This PR adds a warning to that effect and turns off grid search in this situation.
  • It also changes some of the logging messages to actually use the readable learner name instead of the fully qualified class name.
  • It fixes a bug with the custom learner used in the test which came up because there's no reason to do grid search for that majority class learner.

@desilinguist
Turn off grid search when no grids available.
808d274 
- Show a warning.
@desilinguist
Improve error messages
a56ad23 
Show just the name of the model type instead of the fully qualified class name.
@desilinguist
Fix bug in fit() for custom learner used in test
f69f550 
- `fit()` is supposed to return `self` according to sklearn conventions.
@desilinguist
Only show param grids warning if grid search is true.
396cebd
@coveralls
Copy link

coveralls commented Oct 30, 2017 •
edited
Loading

Coverage Status

Coverage increased (+0.08%) to 92.012% when pulling 396cebd on handle-empty-param-grids into efa0a5b on master.

@desilinguist desilinguist self-assigned this Oct 30, 2017
@coveralls
Copy link

coveralls commented Oct 30, 2017 •
edited
Loading

Coverage Status

Coverage increased (+0.009%) to 92.012% when pulling 76cf47c on handle-empty-param-grids into f2064eb on master.

@desilinguist
Copy link
Member Author

This PR is now ready to review (@aoifecahill, @mulhod, @jbiggsets, @benbuleong).

@desilinguist desilinguist added this to the 1.5 milestone Oct 30, 2017
benbuleong
benbuleong approved these changes Oct 30, 2017
View reviewed changes
Copy link
Member

@benbuleong benbuleong left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me overall. I have no other comments/suggestions except the one I just entered for sake of clarity.

doc/run_experiment.rst
which ones to tune and how depend on the data being used for the
parameter grids in SKLL either because either there are no
hyper-parameters to tune or decisions about which parameters
to tune (and how) depend on the data being used for the
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be clearer to actually provide a brief direction where the user can do this tuning? If this "best left up to the user" step is already documented in the user manual, I think it's fine. If not, perhaps cross reference the class/function where this manual tuning is can be done?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By "best left up to the user", I just meant that the user should provide his/her own param_grids in the config file.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the clarification.

@desilinguist desilinguist merged commit 28a80b1 into master Oct 30, 2017
@desilinguist desilinguist deleted the handle-empty-param-grids branch October 30, 2017 18:34
@desilinguist desilinguist mentioned this pull request Oct 30, 2017
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@benbuleong benbuleong benbuleong approved these changes

@jbiggsets jbiggsets jbiggsets approved these changes

@aoifecahill aoifecahill aoifecahill approved these changes

@mulhod mulhod Awaiting requested review from mulhod

Assignees

@desilinguist desilinguist

Labels
None yet
Projects
None yet
Milestone
1.5
Development

Successfully merging this pull request may close these issues.
5 participants
@desilinguist @coveralls @aoifecahill @benbuleong @jbiggsets