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

Fix Kinetics dataset docstring #8121

Merged
merged 2 commits into from
Nov 20, 2023
Merged

Conversation

NicolasHug
Copy link
Member

@NicolasHug NicolasHug commented Nov 18, 2023

The docstring is using the term "clip" instead of "video".

In the context of the video datasets / io / samplers:

  • a video is a video file
  • a clip is a part of a video file

cc @pmeier

Copy link

pytorch-bot bot commented Nov 18, 2023

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/vision/8121

Note: Links to docs will display an error until the docs builds have been completed.

✅ You can merge normally! (2 Unrelated Failures)

As of commit 365823c with merge base 4433680 (image):

FLAKY - The following jobs failed but were likely due to flakiness present on trunk:

This comment was automatically generated by Dr. CI and updates every 15 minutes.

@@ -152,13 +152,13 @@ def _compute_frame_pts(self) -> None:
with tqdm(total=len(dl)) as pbar:
for batch in dl:
pbar.update(1)
clips, fps = list(zip(*batch))
batch_pts, batch_fps = list(zip(*batch))
Copy link
Member Author

@NicolasHug NicolasHug Nov 19, 2023

Choose a reason for hiding this comment

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

This is just renaming of local variables. The main thing is: the variable that used to be called clips really really should not have been called clips. These are pts (presentation time stamps) for each frame in the videos. Nothing to do with a clip in this VideoClips context.

Copy link
Collaborator

@pmeier pmeier left a comment

Choose a reason for hiding this comment

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

@NicolasHug
Copy link
Member Author

It's a pre-existing failure: https://hud.pytorch.org/pr/pytorch/vision/7990

I saw it earlier last week, but couldn't match it to a change in torch core. I was hoping it would just go away but we might need look into it more seriously. In any case, it's not related to this PR.

Copy link
Collaborator

@pmeier pmeier left a comment

Choose a reason for hiding this comment

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

Thanks!

@NicolasHug NicolasHug merged commit c7e2947 into pytorch:main Nov 20, 2023
62 of 64 checks passed
@NicolasHug NicolasHug deleted the aeljfnalefnlajnf branch November 20, 2023 11:54
facebook-github-bot pushed a commit that referenced this pull request Jan 4, 2024
Reviewed By: vmoens

Differential Revision: D52539009

fbshipit-source-id: 7631bf6d4de0c144d581eb324944f14163f4d5e5
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants