-
Notifications
You must be signed in to change notification settings - Fork 7.1k
Add FlyingChairs dataset for optical flow #4860
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
Conversation
💊 CI failures summary and remediationsAs of commit 80badf2 (more details on the Dr. CI page):
1 failure not recognized by patterns:
This comment was automatically generated by Dr. CI (expand for details).Please report bugs/suggestions to the (internal) Dr. CI Users group. |
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.
A few minor comments inline. Apart from them, you also need to add the dataset to the documentation. Otherwise LGTM, thanks @NicolasHug!
@@ -1920,7 +1917,8 @@ def test_flow(self): | |||
with self.create_dataset(split="train") as (dataset, _): | |||
assert dataset._flow_list and len(dataset._flow_list) == len(dataset._image_list) | |||
for _, _, flow in dataset: | |||
assert flow == self._FAKE_FLOW | |||
assert flow.shape == (2, self.FLOW_H, self.FLOW_W) |
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.
Instead of passing this information through class variables, inject_fake_data
can also return a dictionary. If you return dict(num_examples=..., flow_shape=...)
there, you can access it here with
with self.create_dataset(...) as (dataset, info):
...
assert flow.shape == (2, *info["flow_shape"])
if split not in ("train", "val"): | ||
raise ValueError("split must be either 'train' or 'val'") |
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.
Nit: use
vision/torchvision/datasets/utils.py
Lines 442 to 447 in eb48a1d
def verify_str_arg( | |
value: T, | |
arg: Optional[str] = None, | |
valid_values: Iterable[T] = None, | |
custom_msg: Optional[str] = None, | |
) -> T: |
if split not in ("train", "val"): | |
raise ValueError("split must be either 'train' or 'val'") | |
verify_str_arg("split", split, ("train", "val")) |
class FlyingChairs(FlowDataset): | ||
"""`FlyingChairs <https://lmb.informatik.uni-freiburg.de/resources/datasets/FlyingChairs.en.html#flyingchairs>`_ Dataset for optical flow. | ||
|
||
You will also need to download the FlyingChairs_train_val.txt file from the dataset page. |
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.
Is not providing download functionality for these datasets intentional?
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.
For this one specifically and for Sintel, I think we could provide download functionalities. I'm just leaving this for another potential PR
For the others it's not as easy because e.g. FlyingThings3D can only be downloaded via bittorrent, and for Kitti one need to register on the website first (in theory...).
Reviewed By: kazhang Differential Revision: D32216687 fbshipit-source-id: 5f59a29d4f066fa84ad6495a15455ee966a23eb8
This PRs adds the FlyingChairs dataset for optical flow
It also adds a proper way of testing the .flo reader, instead of relying on a mock as previouly done
cc @pmeier