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

Set default value of grid_search in config files to be True. #465

Merged
merged 9 commits into from Feb 15, 2019
Merged

Set default value of grid_search in config files to be True. #465

merged 9 commits into from Feb 15, 2019

Conversation

desilinguist
Copy link
Member

In order to be consistent with the API, it makes sense to set grid_search to be True by default in the config files.

  • Set default value of grid_search to be True in the config parser.
  • Update error message when no objectives are provided to be more informative in light of this similar to the API error message.
  • Move the grid search + learning curve check to config parsing instead of in experiments.py since it belongs here. And update the warning message to be more accurate.
  • Update all tests and templates
    • If we were not previously specifiying grid search, we wanted it to be false so we need to be explicit about that now.
    • Explicitly specify grid search in API everywhere
  • Fix some comments and docstrings in tests.
  • Minor flake8 fixes.

@desilinguist
Make grid_search true by default for config files
9deeb6f 
- Update error message when no objectives are provided  to be more informative in light of this similar to the API error message.
- Move the grid search + learning curve check to conffig parsing instead of in experiments.py since it belongs here.
@desilinguist
Update all tests and templates
1b8282e 
- If we were not previously specifiying grid search, we wanted it to be false so we need to be explicit about that now.
- Explicitly specify grid search in API everywhere
-
@desilinguist
Merge branch 'master' into grid-search-api-config
248b0a3
@desilinguist
Fix failing tests
496acbd 
- Fix stupid typo in objective
- Fix test in accordance with grid search being true by default
@desilinguist
Update warning and fix test.
383ad7e
@desilinguist
Tweak test
44e7f4e
@desilinguist desilinguist added this to the 2.0 milestone Feb 15, 2019
@desilinguist desilinguist self-assigned this Feb 15, 2019
@desilinguist desilinguist added this to In progress in SKLL Release v2.5 via automation Feb 15, 2019
@coveralls
Copy link

Coverage Status

Coverage remained the same at 93.448% when pulling 44e7f4e on grid-search-api-config into b2b5b22 on master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage remained the same at 93.448% when pulling 44e7f4e on grid-search-api-config into b2b5b22 on master.

@coveralls
Copy link

coveralls commented Feb 15, 2019 •
edited

Coverage Status

Coverage increased (+0.2%) to 93.63% when pulling e2993f3 on grid-search-api-config into b2b5b22 on master.

@desilinguist desilinguist modified the milestone: 2.0 Feb 15, 2019
mulhod
mulhod approved these changes Feb 15, 2019
View reviewed changes
Copy link
Contributor

@mulhod mulhod left a comment

Choose a reason for hiding this comment

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

Couple minor comments and nitpicks, but LGTM!

tests/test_cv.py Show resolved Hide resolved
tests/test_input.py Outdated Show resolved Hide resolved
tests/test_input.py Outdated Show resolved Hide resolved
tests/test_input.py Show resolved Hide resolved
tests/test_input.py Outdated Show resolved Hide resolved
ghost
ghost approved these changes Feb 15, 2019
View reviewed changes
Copy link

@ghost ghost 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.

tests/test_classification.py Show resolved Hide resolved
jbiggsets
jbiggsets approved these changes Feb 15, 2019
View reviewed changes
Copy link
Collaborator

@jbiggsets jbiggsets 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!

@desilinguist
Turn grid_search on by default for cross-validation
986522e 
- Raise an error early on if the objective is missing. Don't wait until `train()` is called for one of the folds.
@desilinguist
Update test to be more comprehensive
e6a9348 
- test for both training and cross-validation
@desilinguist
Fix typos.
e2993f3
@desilinguist desilinguist merged commit 5481d0c into master Feb 15, 2019
SKLL Release v2.5 automation moved this from In progress to Done Feb 15, 2019
@desilinguist desilinguist deleted the grid-search-api-config branch February 15, 2019 19:45
@desilinguist desilinguist mentioned this pull request Feb 15, 2019
Closed
@desilinguist desilinguist removed this from Done in SKLL Release v2.5 Sep 12, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mulhod mulhod mulhod approved these changes

@Lguyogiro Lguyogiro Lguyogiro approved these changes

@jbiggsets jbiggsets jbiggsets approved these changes

Assignees

@desilinguist desilinguist

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@desilinguist @coveralls @mulhod @Lguyogiro @jbiggsets