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

Indexing 4D FrameBatch now returns FrameBatch #296

Merged
merged 2 commits into from
Oct 29, 2024

Conversation

NicolasHug
Copy link
Member

@NicolasHug NicolasHug commented Oct 25, 2024

Previously it returned a Frame.

Closes #288, follow-up from #283 (comment)

I don't have a strong preference on this, I see value with both options. I'm opening this PR so we can decide.

The good news is that this change does not require any change to the samplers test, which means the UX should overall be largely unchanged.

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Meta Open Source bot. label Oct 25, 2024
Copy link
Contributor

@ahmadsharif1 ahmadsharif1 left a comment

Choose a reason for hiding this comment

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

I like this change because it makes FrameBatch consistent with Tensor itself.

Should we have an .item() method similar to Tensor to get the underlying Frame for a single-element FrameBatch?

@NicolasHug
Copy link
Member Author

I'm a bit meh on exposing item(). On a tensor, .item() is meant to return a scalar value, so it's pretty different.
I don't think we even need a method for that, creating a Frame from a 3D FrameBatch is as trivial as

Frame(data=fb.data, pts_seconds=fb.pts_seconds, duration_seconds=fb.duration_seconds)

@scotts
Copy link
Contributor

scotts commented Oct 28, 2024

I have a slight preference for this change over the existing code, mainly because this new code is conceptually simpler: iterating over an N-dimensional FrameBatch always yields an N-1-dimensional FrameBatch. I feel that's easier to reason about and document.

If users end-up writing their own code to "if N == 3, turn into Frame" then we can revisit. On .item(), that's the kind of thing I'd want to see more use of before adding. In general, I don't have a strong preference here because I'm just not sure how it will be used. Absent not knowing how it will be used, I fall back on what seems simpler.

@NicolasHug NicolasHug merged commit d1b3daf into pytorch:main Oct 29, 2024
40 checks passed
@NicolasHug NicolasHug deleted the framebatch3d branch October 29, 2024 11:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Meta Open Source bot.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Indexing a 4D FrameBatch: Should this return a Frame or a 3D FrameBatch?
4 participants