Skip to content

Conversation

NicolasHug
Copy link
Contributor

@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
Contributor 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 meta-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