[torchcodec] refactor test utils to be based around dataclasses #57
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
facebook-github-bot
added
the
CLA Signed
|
This pull request was exported from Phabricator. Differential Revision: D59161329 |
|
This pull request was exported from Phabricator. Differential Revision: D59161329 |
3 similar comments
|
This pull request was exported from Phabricator. Differential Revision: D59161329 |
|
This pull request was exported from Phabricator. Differential Revision: D59161329 |
|
This pull request was exported from Phabricator. Differential Revision: D59161329 |
Summary: Pull Request resolved: #57 Refactors our testing utilities to consider the actual reference file to be a first-class concept by making it an object, and then all operations we do are on that object. Some principles I was trying to keep: 1. There should be one clear definition of a reference media file, with all of its important parameters in one place with names that have obvious semantic meaning. 2. Operations that are conceptually connected to a reference media file should be methods on that object. 3. Tests should only have to use the object defined in 1. 4. Formalize the patterns we've already established in how we name reference files. 5. Make adding new reference files easy and obvious. Right now, we can only support reference tensors by timestamp with a generic approach. I think we should try to make that more an explicit pattern based on the pts value, but I'm not sure exactly how to do that right now. That will probably require changing some of our current reference file names. This diff also addresses the awkwardness in how we numbered our reference frames: the first 10 frames used to be 1-index based, and the rest 0-index based. This diff makes all frames 0-index based. Also in the future, the reference file *generation* should probably use these definitions as well. That will ensure we keep everything consistent. Differential Revision: D59161329
|
This pull request was exported from Phabricator. Differential Revision: D59161329 |
|
This pull request has been merged in 7af94c7. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary:
Refactors our testing utilities to consider the actual reference file to be a first-class concept by making it an object, and then all operations we do are on that object. Some principles I was trying to keep:
Right now, we can only support reference tensors by timestamp with a generic approach. I think we should try to make that more an explicit pattern based on the pts value, but I'm not sure exactly how to do that right now. That will probably require changing some of our current reference file names.
Also in the future, the reference file generation should probably use these definitions as well. That will ensure we keep everything consistent.
Differential Revision: D59161329