Remove default value of objective function and `objective` config field. #458

desilinguist · 2019-02-11T22:16:01Z

Remove objective field and remove default values for `objectives.
Remove code for normalizing and error checking the two fields since there is now only one.
Add an additional check that objectives is not empty when running an experiment.
Tweak parallelization to deal with empty objectives, e.g., when not doing grid search.
Make default value of grid_objective to be None for train() and cross_validate() and add a check in train() that raises an exception if we are doing grid search and the objective is not specified. We don't need a similar check for cross_validate() since that calls train() underlyingly anyway.
Make metric a required parameter for learning_curve().
Add new test for empty objectives and remove unnecessary tests e.g., for the removed objective field.
Fix tests that were using objective to now use objectives, explicitly specify functions where needed, and explicitly specify that we are doing grid search.
Fix tests that were relying on score to not use that since that is the objective function value on the test set and will not be computed when there is no such function or when we aren't doing grid search.
Ignore grid_objectives if we aren't doing grid search since this would prevent unnecessary computation when grid search is False but the user still specified a list of objectives.
Add new tests.
Flake8 formatting fixes in several places.

Please test this quite throughly as this is a major backwards incompatible change.

- Remove code for normalizing and error checking the two since there is now only one field. - Add an additional check that `objectives` is not empty when running an experiment.

- Make default value of `grid_objective` to be `None` for `train()` and `cross_validate()` and add a check in `train()` that raises an exception if we are doing grid search and the objective is not specified. - Make `metric` a required parameter for `learning_curve()`. - flake8 fixes.

- No longer need test for the removed `objective` field. - Fix other tests that were using `objective` to now use `objectives`. - flake8 fixes

- Since there's no longer a default value. - flake8 fixes.

- We only need to check this for specific tasks.

- To check that missing objectives is fine for this task.

- Since there are no default objectives now.

- We don't want the objective in the name of the output files if (a) it was not specified to begin with or (b) if there was only one objective since then it's obvious which file is which.

- It will not be available if we aren't doing grid search

- This would prevent unnecesary computation in cases when grid search was false but the user still specified a list of objectives.

- replace 'objective' with 'objectives' - explicitly specify grid search in places - do not rely on default values of objectives - shorter docstrings

- Specify `grid_objective` explicitly when calling `train()`. - Some flake8 fixes

- For some reason the same file was being appended to.

coveralls · 2019-02-11T22:46:36Z

Coverage decreased (-0.08%) to 92.642% when pulling 570d324 on remove-default-objective into 0b13413 on master.

desilinguist · 2019-02-12T15:57:49Z

Something going on with Travis/Coveralls integration again. Re-running builds.

desilinguist · 2019-02-12T17:10:29Z

Yeah, I don't really know why coveralls is reporting 3 new lines as being uncovered. I don't see an issue since that function is called multiple times in learning curve tests.

mulhod · 2019-02-12T17:43:45Z

Even going back to the last build with respect to _train_and_score, it seems like the function is not covered, at least according to Coveralls. I would guess that it claims this because when it's used it is called via parallel(joblib.delayed(_train_and_score)....

desilinguist · 2019-02-12T17:44:10Z

yeah that's what I was thinking as well @mulhod 👍

mulhod

Looks great! I peeked into the coverage "issue" also and, while I suspect the reason the uncovered function is uncovered due to parallelization, I could not figure out why Coveralls would suddenly be claiming 3 lines as newly uncovered. ¯\(ツ)/¯

skll/config.py

jbiggsets

This looks good to me! Two extremely minor comments.

desilinguist added 28 commits February 7, 2019 16:39

Remove objective field and remove default values for objectives

9bfd068

- Remove code for normalizing and error checking the two since there is now only one field. - Add an additional check that `objectives` is not empty when running an experiment.

Fix a comment and fix flake8 issues.

42aa09d

Add new test for empty objectives and remove unnecessary tests

1840b99

- No longer need test for the removed `objective` field. - Fix other tests that were using `objective` to now use `objectives`. - flake8 fixes

Explicitly specify an objective for tests

826d10d

- Since there's no longer a default value. - flake8 fixes.

flake8 fix

352596a

Update all templates to replace objective with objectives.

9ea1fe4

Tweak parallelization to deal with empty objectives.

cfdbbc6

Do not use 'score' in tests for no grid search.

022c25f

Move the error checking for missing grid objectives

7b42998

- We only need to check this for specific tasks.

Add new test for learning curve task

acdfe40

- To check that missing objectives is fine for this task.

Fix test that previously passed to raise error

f4c558a

- Since there are no default objectives now.

grid_objective -> grid_objectives everywhere.

505a5fd

We need two conditions not just one

24d5552

- We don't want the objective in the name of the output files if (a) it was not specified to begin with or (b) if there was only one objective since then it's obvious which file is which.

Only print 'score' when available

bed25a8

- It will not be available if we aren't doing grid search

Ignore grid_objectives if we aren't doing grid search

5d02977

- This would prevent unnecesary computation in cases when grid search was false but the user still specified a list of objectives.

Make docstring for test shorter

3a321ff

Shorter docstring

7085863

Replace 'score' with 'accuracy' for no grid search

876344b

Fix tests

3fccea6

- replace 'objective' with 'objectives' - explicitly specify grid search in places - do not rely on default values of objectives - shorter docstrings

Remove unnecessary file.

ffa4b52

Add new test for no grid search with list of objectives

b4a1709

Add explicit objectives to test

9499aa3

Several flake8 fixes

d7cf274

Do not rely on 'score'

8fc66ae

- Specify `grid_objective` explicitly when calling `train()`. - Some flake8 fixes

Fix silly teardown issue

8603e14

- For some reason the same file was being appended to.

Revert typo from earlier commit.

5d94271

Merge branch 'master' into remove-default-objective

07c4598

desilinguist requested review from a user and mulhod February 11, 2019 22:16

desilinguist requested review from Lguyogiro, maxwell-schwartz and jbiggsets February 11, 2019 22:16

desilinguist self-assigned this Feb 11, 2019

desilinguist added this to In progress in SKLL Release v2.5 via automation Feb 11, 2019

desilinguist added this to the 2.0 milestone Feb 11, 2019

Add new test to check for bad learning curve cv folds

571b858

mulhod approved these changes Feb 12, 2019

View reviewed changes

jbiggsets reviewed Feb 13, 2019

View reviewed changes

skll/config.py Show resolved Hide resolved

jbiggsets reviewed Feb 13, 2019

View reviewed changes

skll/config.py Outdated Show resolved Hide resolved

jbiggsets approved these changes Feb 13, 2019

View reviewed changes

Show offending type in TypeError.

570d324

desilinguist merged commit 1067873 into master Feb 13, 2019

SKLL Release v2.5 automation moved this from In progress to Done Feb 13, 2019

desilinguist deleted the remove-default-objective branch February 13, 2019 15:18

This was referenced Feb 13, 2019

Remove default tuning objective and make it a required field. #381

Closed

get warning with learning curves unnecessarily #410

Closed

Write new tests to increase test coverage #462

Merged

desilinguist removed this from Done in SKLL Release v2.5 Sep 12, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove default value of objective function and `objective` config field. #458

Remove default value of objective function and `objective` config field. #458

desilinguist commented Feb 11, 2019

coveralls commented Feb 11, 2019 •

edited

desilinguist commented Feb 12, 2019

desilinguist commented Feb 12, 2019 •

edited

mulhod commented Feb 12, 2019

desilinguist commented Feb 12, 2019

mulhod left a comment •

edited

jbiggsets left a comment

Remove default value of objective function and objective config field. #458

Remove default value of objective function and objective config field. #458

Conversation

desilinguist commented Feb 11, 2019

coveralls commented Feb 11, 2019 • edited

desilinguist commented Feb 12, 2019

desilinguist commented Feb 12, 2019 • edited

mulhod commented Feb 12, 2019

desilinguist commented Feb 12, 2019

mulhod left a comment • edited

Choose a reason for hiding this comment

jbiggsets left a comment

Choose a reason for hiding this comment

Remove default value of objective function and `objective` config field. #458

Remove default value of objective function and `objective` config field. #458

coveralls commented Feb 11, 2019 •

edited

desilinguist commented Feb 12, 2019 •

edited

mulhod left a comment •

edited