Ensure videos are allocated into all specified splits #169
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Codecov Report
@@ Coverage Diff @@
## master #169 +/- ##
======================================
Coverage 85.0% 85.0%
======================================
Files 30 30
Lines 1843 1851 +8
======================================
+ Hits 1567 1575 +8
Misses 276 276
|
pjbull
requested changes
Dec 13, 2021
pjbull
reviewed
Dec 13, 2021
|
@pjbull ready for another look. this now has the added benefit of doing split allocation within species which is something we had desired but hadn't yet implemented. this should help ensure better training results (in addition to fixing the bug) |
|
@pjbull ready for reals this time! |
pjbull
reviewed
Dec 14, 2021
| list(values["split_proportions"].keys()), | ||
| weights=list(values["split_proportions"].values()), | ||
| k=len(species_df) - len(expected_splits), | ||
| ) |
There was a problem hiding this comment.
This has one weird edge case that is likely rare, so just worth filing an issue for:
v0.mp4, antelope
v1.mp4, antelope # v1 assigned test by antelope grouping
v1.mp4, cow # subsequently v1 assigned train by cow grouping
v2.mp4, antelope
v4.mp4, cow
v5.mp4, cow
# test set is now missing antelope
pjbull
approved these changes
Dec 14, 2021
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR fixes the issue that caused the following error:
The issue has to do with the
splitcolumn. When we don't provide it in the labels, we use a random split. But we don't guarantee that we have all of the values from the split_proportions dict (e.g.trainandval) in the split column. With a small number of videos and the default proportions, all videos getting assigned to train (see insplits.csv). Since there are no val videos, there is noval_f1_score.I considered implementing this without the random draw and instead calculating the number of samples that need to go into each split based on the proportions provided. However, this gets tricky to ensure based on rounding. I instead went with the while loop that tries different seeds until it finds a draw that works. If someone specifies split proportions of
train: 100, val: 1, and only provides say 5 videos, there is risk of an infinite loop. I'm not sure how important it is to catch such a situation since 1) we don't expect people to be training on so few videos and 2) it feels hard to accidentally end up in such a situation.Bonus fix: fix bug in ModelCheckpoint which lets monitor be None if there is no early stopping.
modemust either beminormax. I set it tominto match PTL's default but it doesn't get used if monitor is None.Plus cleanup to set defaults in
DummyTrainConfigto avoid repeated code.