-
Notifications
You must be signed in to change notification settings - Fork 7
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
Make video download optional for sample datasets #224
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #224 +/- ##
=======================================
Coverage 99.70% 99.70%
=======================================
Files 12 12
Lines 681 681
=======================================
Hits 679 679
Misses 2 2 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice one! 🚀
Most comments are on the docs description, trying to make some sentences a bit shorter. But this is often very subjective so of course feel free to take/leave bits as you see fit!
|
||
If available, the video file is downloaded and its path is stored |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If available, the video file is downloaded and its path is stored | |
If available, the video file is downloaded to the path |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this case I think I'll stick with my more verbose phrasing, fro the sake of clarity. But I'll change "e.g." to "i.e".
still frame extracted from the video. This can serve as a representative | ||
image of the video, which could be useful for visualisation (e.g., |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
still frame extracted from the video. This can serve as a representative | |
image of the video, which could be useful for visualisation (e.g., | |
still frame extracted from the video. This can be useful for visualisation (e.g., |
IMO it's not very clear what a "representative image of the video" means so I'd suggest to remove it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(as in, how is it "representative"?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
agreed
|
Thanks for the review @sfmig!. I've accepted most of your suggestion on phrasing. |
Description
What is this PR
Why is this PR needed?
See issue 181.
What does this PR do?
It introduces a
with_video
argument tosample_data.fetch_dataset
andsample_data.fetch_dataset_paths
. The default isFalse
, which is sensible I think because videos can be large and we don't want to get them unless we are sure we want them. This should speedup CI somewhat, and also the execution of examples in binder.There is only one test (so far) for which a video is necessary, the one that tests the video-fetching functionality itself. For that test we now use the smallest available video (~5MB).
The "Sample Data" section of docs has been updated accordingly.
References
Closes #181
How has this PR been tested?
Updated existing test with parametrisation across the new
with_video
argument.Is this a breaking change?
Noe.
Does this PR require an update to the documentation?
Yes, done.
Checklist: