Add regular index-based sampler - part 1 #240
Conversation
facebook-github-bot
added
the
CLA Signed
NicolasHug
changed the title
clips_at_regular_indices() samplerclips_at_regular_indices() sampler
NicolasHug
changed the title
clips_at_regular_indices() sampler
| def clips_at_regular_indices( | ||
| decoder: VideoDecoder, | ||
| *, | ||
| num_clips: int = 1, |
There was a problem hiding this comment.
I feel like clips_at_regular_indices(num_clips=1) is an anti pattern. Same with 2. Maybe we should make 3 the default, and enforce num_clips>=3? Or not enforce, but still set the default to 3?
I don't feel too strongly about it either way - just being pedantic.
There was a problem hiding this comment.
If this is what other libraries do we can do the same here.
I was thinking of sampling the middle of the video if num_clips=1 but that would make it slower than existing libraries
There was a problem hiding this comment.
OK. Other libs allow num_clips=1. Let's revisit if needed
| def clips_at_regular_indices( | ||
| decoder: VideoDecoder, | ||
| *, | ||
| num_clips: int = 1, |
There was a problem hiding this comment.
If this is what other libraries do we can do the same here.
I was thinking of sampling the middle of the video if num_clips=1 but that would make it slower than existing libraries
| assert len(torch.unique(clip_starts_seconds)) == sampling_range_size | ||
|
|
||
| # Assert clips starts are ordered, i.e. the start indices don't just "wrap | ||
| # around". They're duplicated *and* ordered. |
There was a problem hiding this comment.
Mention reason for not wrapping around?
There was a problem hiding this comment.
There's no particular reason honestly, it's just an assertion of the behavior of torch.linspace. I'll add a comment near the Note to clarify that this is somewhat arbitrary.
This PR adds an index-based sampler that returns equally-spaced clips.
It is a partial implementation, just like the random sampler in #221. In particular it is missing 2 key parts:
Both these parts are left as TODO and are tracked in #239. I am hoping to merge this PR before we tackle those, because I want to make sure the solution we come up with will satisfy both samplers (and not just the existing random sampler).