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.

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

Bugfix/cv folds file logging #365

Merged
merged 4 commits into from
Sep 14, 2017
Merged

Conversation

Lguyogiro
Copy link
Contributor

  • Addresses issue Weird logging issue when specifying cv_folds_file #349 . Previously, cv_folds was assumed to be an int. But if "cv_folds_file" is specified in the config, cv_folds will be a dictionary mapping ids to fold numbers and requires new code to correctly log the number of folds.
  • Fixed by checking the type of cv_folds and, if it is not an int, logging the number of unique fold ids from the dict values.
  • Added a cv folds file to tests/train/ and test_cv_folds_file_logging to test suite

@coveralls
Copy link

coveralls commented Sep 13, 2017

Coverage Status

Coverage decreased (-0.03%) to 91.251% when pulling 9e308a9 on bugfix/cv_folds_file-logging into ac4df66 on master.

@coveralls
Copy link

coveralls commented Sep 14, 2017

Coverage Status

Coverage increased (+0.6%) to 91.865% when pulling d17f328 on bugfix/cv_folds_file-logging into ac4df66 on master.

Copy link
Member

@desilinguist desilinguist 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. Thanks @Lguyogiro!

@Lguyogiro
Copy link
Contributor Author

I've pushed one more minor change: previously the experiment name (in the config file) for testing the cv folds file logging was "test_save_cv_folds". I've updated to reflect what it is actually testing.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.5%) to 91.788% when pulling c816bdc on bugfix/cv_folds_file-logging into ac4df66 on master.

@coveralls
Copy link

coveralls commented Sep 14, 2017

Coverage Status

Coverage increased (+0.6%) to 91.865% when pulling c816bdc on bugfix/cv_folds_file-logging into ac4df66 on master.

@desilinguist
Copy link
Member

Since this is a very simple change, I am just going to merge it now without waiting for another approval.

@desilinguist desilinguist reopened this Sep 14, 2017
@desilinguist desilinguist merged commit edae90a into master Sep 14, 2017
@desilinguist desilinguist deleted the bugfix/cv_folds_file-logging branch September 14, 2017 19:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants