Skip to content

Update RAFT Stereo to be more sync with CREStereo implementation #6575

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

Merged
merged 10 commits into from
Sep 14, 2022

Conversation

YosuaMichael
Copy link
Contributor

After discussing with @TeodorPoncu for on the CREStereo implementation, we plan to make the implementation more consistent. Here are the changes in RAFT Stereo:

  1. Change the input name from image1 and image2 to left_image and right_image
  2. Enable using flow_init as input
  3. Have self.output_channel as variable in RAFTStereo class

Here is the link to CREStereo implementation PR: #6310

Copy link
Contributor

@TeodorPoncu TeodorPoncu left a comment

Choose a reason for hiding this comment

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

LGTM, just one question such that I know how to sync CREStereo to these changes.

Copy link
Contributor

@datumbox datumbox left a comment

Choose a reason for hiding this comment

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

LGTM, I think the changes make sense and make the code much easier to read. Coming with basic knowledge of Depth Perception, the new naming allows me to understand what is happening and why.

Comment on lines +459 to +460
if flow_init is not None:
coords1 = coords1 + flow_init
Copy link
Contributor

Choose a reason for hiding this comment

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

This is the only change for which I don't have context, but I understand it's a new feature.

Copy link
Contributor

@TeodorPoncu TeodorPoncu Sep 13, 2022

Choose a reason for hiding this comment

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

When wanting to perform inference at a resolution significantly larger than that at which the model is trained, you can perform cascaded inference.

Cascaded inference first computes the flow for a downsampled version of the image, and uses that flow as a prior for the full resolution image.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just to give more context:
Here is the code for the cascaded inference: https://github.com/pytorch/vision/blob/test-crestereo-training/references/stereo_matching/evaluation.py#L57

And here is reference to original raft implementation on this part: https://github.com/princeton-vl/RAFT-Stereo/blob/main/core/raft_stereo.py#L104

@oke-aditya
Copy link
Contributor

My doubt is do stereo models depend on order left and right input images. Can we not reverse them?
Like can we call the same model by changing left image to right and right image to left and still expect same output?

@YosuaMichael
Copy link
Contributor Author

YosuaMichael commented Sep 13, 2022

@oke-aditya no we can't reverse the input since what the model does is guessing how much "displacement" of the pixels from left_image to the right_image, and this usually mean all pixels will be shifted to the right (not the other way around).

We have however augmentation that do horizontal flip, and in this case we will also flip the order of right and left image.

@oke-aditya
Copy link
Contributor

Perhaps we can document this.

@YosuaMichael
Copy link
Contributor Author

YosuaMichael commented Sep 13, 2022

@oke-aditya do you have any recommendation on where we should document it? I think this behavior will be general on all depth stereo model (currently we also plan to add CREStereo). Maybe we can add on the README when we add the references?

@oke-aditya
Copy link
Contributor

Probably one root page for stereo models ? Also cc @datumbox for better ideas 💡

@YosuaMichael
Copy link
Contributor Author

The test error is not related to this PR changes

@YosuaMichael YosuaMichael merged commit 355b278 into pytorch:main Sep 14, 2022
facebook-github-bot pushed a commit that referenced this pull request Sep 15, 2022
…tion (#6575)

Summary:
* Update raft_stereo to sync forward param with CREStereo and have output_channel

* Add flow_init param to docstring

* Use output_channels instead of output_channel

* Replace depth with disparity since what we predict actually disparity instead of actual depth

Reviewed By: jdsgomes

Differential Revision: D39543287

fbshipit-source-id: af1ba127fb25eedd3618d6f7b57565daf04ad986
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants