Speed up the use of an external folds file #367

desilinguist · 2017-10-12T18:08:42Z

Fix the bug that was causing the inner grid-search loop of a cross-validation experiment to use a single job instead of the number specified via grid_search_jobs.
Rename the cv_folds_file config file option to folds_file and update the documentation to reflect that it is applicable to both the training and the cross-validation experiments..
Add a new config file option called use_folds_file_for_grid_search which controls whether the inner-loop grid-search uses the folds specified via folds_file. It's set to True by default which means. Setting it to False means that the inner loop uses the value of grid_search_folds (3 by default). This option is only used in case of cross-validation experiments and is ignored otherwise. Add documentation for this option.
Also add a new keyword argument called use_custom_folds_for_grid_search to the learner.cross_validate() method. This flag by default sets grid_search_folds to be the same as cv_folds if custom folds are specified. It can be set to False to prevent this. This is required to handle the case when cross_validate() is directly called via the API, by passing the configuration parser.
Add a new comprehensive test to test the setting the values of cv_folds and grid_search_folds in the configuration file based on the above options.
Refactor tests to deal with the renaming of cv_folds_file to folds_file.
Add documentation for the save_cv_folds option which we had forgotten to do when the option was added back in the day.
Minor re-indentation of some part of the code for readability.

(Note: See #363 for detailed timing experiments.)

1. Fix bug that led to using only a single job when CV folds were explicitly specified in local mode. 2. Change `MAX_CONCURRENT_PROCESSES` to 3 since that's what we use as the default number of grid jobs. This would also fix the issue when we were requesting more slots than necessary on the grid.

- Add a new `use_folds_file_for_grid_search` option that lets cross-validation experiments with specified folds bypass using the specified folds for the inner grid-search loop. - Refactor and rename some variables to make things cleaner.

- Pass through `use_folds_file_for_grid_search` in case we need to use it. - Reindent some long lines.

- Both `grid_search_folds` and `cv_folds` are now used as passed in from the config file. - Reindent and refactor some long lines.

- Test both `grid_search_folds` and `cv_folds` for both the `train` and `cross_validate` tasks. - Required some minor changes to `utils.py` and a template file.

- Only print out grid search related warnings when the user actually wants to do grid search.

- Update documentation for `folds_file` (formerly `cv_folds_file`). - Add documentation for new parameter `use_folds_file_for_grid_search`. - Add missing documentation for `save_cv_folds`.

coveralls · 2017-10-12T18:35:03Z

Coverage decreased (-0.2%) to 91.629% when pulling d466115 on fix/cv-folds-file-is-slower into 13e4982 on master.

- We want to hit the deprecation warning.

- This flag by default sets `grid_search_folds` to be the same as `cv_folds` if custom folds are specified. It can be set to False to prevent this.

coveralls · 2017-10-13T00:04:50Z

Coverage increased (+0.1%) to 91.977% when pulling 3c30141 on fix/cv-folds-file-is-slower into 13e4982 on master.

desilinguist · 2017-10-13T00:09:43Z

@aoifecahill @dan-blanchard @Lguyogiro @aloukina this is now ready for review! The reason you can't see the checks is because I pushed a docstring change in the last commit and didn't want to build the whole thing again because of that. The green check mark in the last commit and the coverage increase should be all you need :)

dan-blanchard

Looks good to me

dan-blanchard · 2017-10-13T13:08:54Z

skll/config.py

@@ -455,7 +455,7 @@ def _parse_config_file(config_path):
    random_folds = config.getboolean("Input", "random_folds")
    if random_folds:
        if folds_file:
-            logger.warning('Specifying folds_file overrides random_folds')
+            logger.warning('Specifying \"folds_file\" overrides \"random_folds\".')


Nitpick: No need for the slashes on these quotes, since it's " inside '.

aloukina

I mainly have documentation comments and I highlighted a few situations where we should print a warning.

aloukina · 2017-10-13T16:41:46Z

doc/run_experiment.rst

+keep the same folds for both the outer and the inner cross-validation loops.
+
+However, sometimes the goal of specifying the folds file is simply for the
+purposes of comparison to another existing experiment or another context


aloukina · 2017-10-13T16:43:54Z

doc/run_experiment.rst

+
+Whether to use the specified :ref:`folds_file` for the inner grid-search
+cross-validation loop when :ref:`task` is set to ``cross_validate``. Ignored
+for all other tasks. Defaults to ``True``.


Perhaps add a note that this will always be set to True for train?

aloukina · 2017-10-13T16:44:18Z

doc/run_experiment.rst

+
+save_cv_folds *(Optional)*
+""""""""""""""""""""""""""
+


Which loop folds would these be?

Whatever folds end up being used.

So if I have a cross-validate task, would these be outer or inner loop?

This is always the outer loop - the ones referred to by cv_folds.

aloukina · 2017-10-13T16:52:39Z

tests/test_input.py

+    # ('train', None, 7, None) ->  (None, 7)
+    # ('train', None, 7, True) ->  (None, 7)
+    # ('train', None, 7, False) ->  (None, 7)
+    # ('train', 5, None, None) ->  (None, 3)


Is there a warning in this case?

Why would there need to be a warning?

You ask for 5 folds but your request is ignored since you asked in the wrong place and you get 3.

Yeah, this is an issue of not reading the documentation so I personally think this is a user problem. PEBKAC :-p

aloukina · 2017-10-13T16:52:55Z

tests/test_input.py

+    # ('train', None, 7, True) ->  (None, 7)
+    # ('train', None, 7, False) ->  (None, 7)
+    # ('train', 5, None, None) ->  (None, 3)
+    # ('train', 5, None, True) ->  (None. 3)


aloukina · 2017-10-13T16:53:28Z

tests/test_input.py

+    # ('train', 5, None, None) ->  (None, 3)
+    # ('train', 5, None, True) ->  (None. 3)
+    # ('train', 5, None, False) ->   (None, 3)
+    # ('train', 5, 7, None) ->  (None, 7)


There should be a warning too, I suppose?

Again, not sure why we need a warning.

Because your request for 5 is silently ignored.

The 5 doesn't matter here because that's the value for cv_folds which is not used for train at all.

aloukina · 2017-10-13T16:53:45Z

tests/test_input.py

+    # ('train', 'train/folds_file_test.csv', None, None) ->  (None, fold_mapping)
+    # ('train', 'train/folds_file_test.csv', None, True) ->  (None, fold_mapping)
+    # ('train', 'train/folds_file_test.csv', None, False) ->  (None, fold_mapping)
+    # ('train', 'train/folds_file_test.csv', 7, None) ->  (None, fold_mapping)


Same, your request for 7 is ignored.

Yes, there's a warning for this already.

aloukina · 2017-10-13T16:54:09Z

tests/test_input.py

+    # ('cross_validate', None, None, True) ->  (10, 3)
+    # ('cross_validate', None, None, False) ->  (10, 3)
+    # ('cross_validate', None, 7, None) ->  (10, 7)
+    # ('cross_validate', None, 7, True) ->  (10, 7)


aloukina · 2017-10-13T16:55:19Z

tests/test_input.py

+    # ('cross_validate', None, 7, True) ->  (10, 7)
+    # ('cross_validate', None, 7, False) ->  (10, 7)
+    # ('cross_validate', 5, None, None) ->  (5, 3)
+    # ('cross_validate', 5, None, True) ->  (5, 3)


I would think in this case the user will expect the same folds used for both loops so we should warn them?

aloukina · 2017-10-13T16:55:52Z

tests/test_input.py

+    # ('cross_validate', 'train/folds_file_test.csv', None, None) ->  (fold_mapping, fold_mapping)
+    # ('cross_validate', 'train/folds_file_test.csv', None, True) ->  (fold_mapping, fold_mapping)
+    # ('cross_validate', 'train/folds_file_test.csv', None, False) ->  (fold_mapping, 3)
+    # ('cross_validate', 'train/folds_file_test.csv', 7, None) ->  (fold_mapping, fold_mapping)


@aloukina if you look in the code, I had already added some new warnings in config.py. This is a test, so not sure why we need warnings here?

@aloukina here are the new warnings that are already in the PR (each word is a link).

You don't need them in test of course - it's just easier to see all places with warnings when you have all combinations written out with expected behaviour.
Since you know the code better, you know where the warnings are already in place. For others we could just add an issue to add them in the future.

I checked and I think I already have the warnings where I think they are really important.

- Add note to new flag about train task. - Improve references to other options.

Lguyogiro

LGTM

aoifecahill · 2017-10-16T13:34:05Z

skll/config.py

+    #      then use that mapping for grid search instead of the value
+    #      contained in `grid_search_folds`.
+    #  (b) if the task is `cross_validate` and an external fold mapping is specified and the
+    #      then use that mapping for the outer CV loop. Depending on the value of


- We were not actually passing in the flag to `cross_validate()` as we should be.

- And reorder the condition so that reading it makes more sense.

coveralls · 2017-10-17T14:51:00Z

Coverage increased (+0.1%) to 91.98% when pulling 36f156b on fix/cv-folds-file-is-slower into 13e4982 on master.

aoifecahill

I tested this on one of the datasets that prompted me to file the original issue and observed a noticeable decrease in the time taken to run an experiment. Thanks!

desilinguist added 16 commits October 6, 2017 10:47

Minor changes

598e840

- Pass through `use_folds_file_for_grid_search` in case we need to use it. - Reindent some long lines.

Some refactoring for fold computation.

50d76b4

- Both `grid_search_folds` and `cv_folds` are now used as passed in from the config file. - Reindent and refactor some long lines.

Add comprehensive test for folds computation

3b047e3

- Test both `grid_search_folds` and `cv_folds` for both the `train` and `cross_validate` tasks. - Required some minor changes to `utils.py` and a template file.

Uncomment clean up code and clean up test logs

a0a4bcb

Add a deprecation warning for cv_folds_file.

28ed00d

Redo deprecation checking of cv_folds_file.

8c494a0

Fix missing key error.

ed25c33

Improve logging messages.

82a39db

Only show warnings when necessary

b2d3c5b

- Only print out grid search related warnings when the user actually wants to do grid search.

Update summary test to account for new field.

283503f

Rename cv_folds_file to folds_file everywhere.

d105e44

Update documentation

930cc2b

- Update documentation for `folds_file` (formerly `cv_folds_file`). - Add documentation for new parameter `use_folds_file_for_grid_search`. - Add missing documentation for `save_cv_folds`.

One final cv_folds_file straggler.

28af5ce

No, really last one.

d466115

desilinguist self-assigned this Oct 12, 2017

desilinguist requested review from dan-blanchard, aoifecahill, Lguyogiro and aloukina October 12, 2017 18:08

desilinguist added 4 commits October 12, 2017 17:10

Keep cv_folds_file in one place

a768500

- We want to hit the deprecation warning.

Add a flag to learner.cross_validate().

fc50218

- This flag by default sets `grid_search_folds` to be the same as `cv_folds` if custom folds are specified. It can be set to False to prevent this.

Minor changes to log messages.

3c30141

Improve docstring [ci skip]

a18f16f

dan-blanchard approved these changes Oct 13, 2017

View reviewed changes

aloukina approved these changes Oct 13, 2017

View reviewed changes

desilinguist added 4 commits October 13, 2017 14:01

Do not escape quotes.

cb2f0bd

More updates to documentation

06cacca

- Add note to new flag about train task. - Improve references to other options.

Fix typo in comment [ci skip]

d53a421

Minor tweak to documentation [ci skip]

bf6ac49

Lguyogiro approved these changes Oct 13, 2017

View reviewed changes

aoifecahill reviewed Oct 16, 2017

View reviewed changes

desilinguist added 6 commits October 16, 2017 10:58

fix typo [ci skip]

8ab792e

Fix bug in calling cross_validate()

1567260

- We were not actually passing in the flag to `cross_validate()` as we should be.

Use the correct name for argument [ci skip]

f265a6e

Fix typo in comment.

92f86c6

Add warning inside cross_validate()

99e43ce

- And reorder the condition so that reading it makes more sense.

Only show the warning if we are actually doing grid search.

36f156b

Fix typo [ci skip]

55a336b

aoifecahill approved these changes Oct 17, 2017

View reviewed changes

desilinguist merged commit 0052077 into master Oct 17, 2017

desilinguist deleted the fix/cv-folds-file-is-slower branch October 17, 2017 18:19

desilinguist mentioned this pull request Oct 17, 2017

specifying cross validation folds slows down experiments #363

Closed

desilinguist mentioned this pull request Nov 13, 2017

Mismatch between default grid search folds and number of slots in gridmap #342

Closed


		save_cv_folds (Optional)
		""""""""""""""""""""""""""

Speed up the use of an external folds file #367

Speed up the use of an external folds file #367

Conversation

desilinguist commented Oct 12, 2017 • edited Loading

coveralls commented Oct 12, 2017 • edited Loading

coveralls commented Oct 13, 2017 • edited Loading

desilinguist commented Oct 13, 2017 • edited Loading

dan-blanchard left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aloukina left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

desilinguist Oct 13, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

desilinguist Oct 13, 2017 • edited Loading

Choose a reason for hiding this comment

desilinguist Oct 13, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Lguyogiro left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Oct 17, 2017 • edited Loading

aoifecahill left a comment

Choose a reason for hiding this comment

desilinguist commented Oct 12, 2017 •

edited

Loading

coveralls commented Oct 12, 2017 •

edited

Loading

coveralls commented Oct 13, 2017 •

edited

Loading

desilinguist commented Oct 13, 2017 •

edited

Loading

desilinguist Oct 13, 2017 •

edited

Loading

desilinguist Oct 13, 2017 •

edited

Loading

desilinguist Oct 13, 2017 •

edited

Loading

coveralls commented Oct 17, 2017 •

edited

Loading