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.
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
Add Sintel Dataset to the dataset prototype API #4895
base: main
Are you sure you want to change the base?
Add Sintel Dataset to the dataset prototype API #4895
Changes from 12 commits
eff55d3
61831dc
12b5915
d2dcba9
1b690ac
6f371c7
081c70f
ad74f96
6c106e7
32cc661
7f27e3f
28def28
cdcb914
b58c14b
1d7a36e
52ba6da
e515fbb
7892eb6
ee3c78f
79c65fb
08cd984
591633a
98872fd
7ccca53
8f84b51
709263c
6b40366
34e8de3
cb904c5
0e13b3f
10bdc4b
7b4265f
d34ebe6
54618c6
6c04d5f
84c4e88
ebf7e4a
c0b254c
e9fa656
3724869
69194e1
b4cce90
527d1fa
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
these are expected to be encoded as little endian as well
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a TODO left from the current comments. Working on it now. Thanks @NicolasHug for pointing this out.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I changed the
dtype
fromnp.float32
to<f4
fordata_arr
, but just wanted to quickly check, is this the correct fix, or is there any other way to do this? cc: @pmeierThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes
<f4
would be correct, it's a little endian float that fits in 4 bytes (float32).But it might be preferable to always call the same function instead of mixing
frombuffer
andfrom_bytes
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, that makes sense. Thanks, @NicolasHug! I've fixed this (preferred using
frombuffer
in both instances).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The reason this function exists, and is not used from the existing
read_flo
function (in the other API):zipfile
objects don't work well withnp.fromfile
and there is anio.unsupportedoperation: fileno
error that is raised. Please note that earlier while testing with @pmeier, we didn't get an error because a generator was returned which doesn't give an error unless and until you use it. This error was raised when I replacedyield
withreturn
.I also verified the results from the function used here with this function, and the results are same for a single file (didn't check for all of them).
Suggestions are, as always, welcome. :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh well. @NicolasHug that is a strong argument in favor of #4882 since reading from archives will be the norm for the new datasets.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, if really we can't re-use the original code, then maybe having a helper makes sense. Although I would still advocate for avoiding fancy features / new parameter names as much as possible, and to make the wrapper as thin as possible.
I'd be curious to see speed comparisions between the current version (with unzipped data) and the new one though, if you have time to run a quick benchmark that would be awesome.
Before merging anything we should try to run more robust tests to avoid surprises in the future :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should not add this functionality here, but rather go for #4882 and depend on that here. Of course that would involve writing tests for a
read_flo
or any other wrapper.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Question: What do we want to return here?
image1
should be atuple
of path and buffer? Or should it beimage1_path
andimage1? (same question for
image2and
flow`)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The latter. The dictionary should contain the image, which is either a tensor or a buffer depending on
decoder
, as well the image path.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done! :) Should the label be
flow
here?