Skip to content

Conversation

@desilinguist
Copy link
Collaborator

This PR closes #593.

  • Add a new cv_seed keyword argument to the Learner.cross_validate() method with the same default value as before.
  • Create a new cv_seed configuration file field in the “Input” section. Also, show the seed in the experiment log.
  • Update the documentation to describe this new field.
  • Add a new input test for "cv_seed" field.
  • Add new CV test for the “cv_seed” field
    • Add a new test generator that sets a custom seed and then checks whether the folds generated from inside SKLL match expected folds, for both classifiers and regressors. This also needs two new configuration file templates.
    • This test requires computing the expected folds which we were already doing for another, already existing test. So, I factored that code out into a utility function in tests/utils.py.
  • Allow cv_seed for voting learners too.
    • Add tests for the voting learners side in the same vein as the regular learners using the same utility function.
  • Tweak various existing tests due to this extra field.
  • Tweak CI plans for timing
    • test_voting_learners_api_5.py now takes 2x longer due to the addition of a new boolean field so let's move it to a different test partition which is much shorter than others.

Add a new `cv_seed` keyword argument to `Learner.cross_validate()` method with the same default value as before.
- This field should be in the "Input" section.
- Add it to the documentation.
- Show the seed in the experiment log.
- Add a new test generator that sets a custom seed and then checks whether the folds generated from inside SKLL match expected folds, for both classifiers and regressors.
- This test requires computing the expected folds which we were already doing for another, already existing test. So, I factored that code out into a utility function in `tests/utils.py`.
- Also add tests for the voting learners side in the same vein as the regular learners using the same utility function.
- `test_voting_learners_api_5.py` now takes 2x longer due to the addition of a new boolean field so let's move it to a different test partition which is much shorter than others.
@desilinguist desilinguist requested review from a user, Frost45, damien2012eng and mulhod December 15, 2021 23:35
@codecov
Copy link

codecov bot commented Dec 16, 2021

Codecov Report

Merging #707 (a0dabd6) into main (bfe89bc) will increase coverage by 0.03%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #707      +/-   ##
==========================================
+ Coverage   96.89%   96.92%   +0.03%     
==========================================
  Files          63       63              
  Lines        9199     9263      +64     
==========================================
+ Hits         8913     8978      +65     
+ Misses        286      285       -1     
Impacted Files Coverage Δ
tests/test_voting_learners_expts_1.py 98.38% <ø> (ø)
tests/test_voting_learners_expts_2.py 98.82% <ø> (ø)
tests/test_voting_learners_expts_3.py 98.82% <ø> (ø)
tests/test_voting_learners_expts_4.py 98.76% <ø> (ø)
tests/test_voting_learners_expts_5.py 98.48% <ø> (ø)
skll/config/__init__.py 95.14% <100.00%> (+0.01%) ⬆️
skll/experiments/__init__.py 95.17% <100.00%> (+0.02%) ⬆️
skll/learner/__init__.py 97.13% <100.00%> (ø)
skll/learner/voting.py 98.45% <100.00%> (ø)
tests/test_cv.py 100.00% <100.00%> (ø)
... and 4 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bfe89bc...a0dabd6. Read the comment docs.

Copy link
Contributor

@Frost45 Frost45 left a comment

Choose a reason for hiding this comment

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

Looks great @desilinguist! A few minor suggestions.

@desilinguist
Copy link
Collaborator Author

@Frost45 please take a look at the changes!

Copy link
Contributor

@Frost45 Frost45 left a comment

Choose a reason for hiding this comment

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

Looks great @desilinguist! Thank you for doing this! 🙏🏼

Co-authored-by: Sanjna Kashyap <20379363+Frost45@users.noreply.github.com>
@desilinguist desilinguist merged commit caa9b56 into main Dec 20, 2021
@delete-merged-branch delete-merged-branch bot deleted the 593-allow-specifying-seed-for-xval branch December 20, 2021 04:09
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.

Allow the user to control the random seed for cross validation fold generation

4 participants