Adding detect_flow and detect_flows functions

[sugatoray]

Added the following two functions to allow users easily create optical flow and optionally save the data (as images and as .pt data files respectively).

• detect_flow: detects optical-flow for a single image-pair
• detect_flows: detects optical-flow for a series of image-pairs

The detect_flow() function makes sure of managing memory (grabage collection + emptying cuda cache), while evaluating and saving optical flow information on a pair of images. When I tried running the RAFT demo to create a series of optical-flow images, I found the colab notebook running out of memory. These functions take care of the memory aspect and make it easier for the user to apply optical flow on image-pairs.

Gif Source: LinkedIn Post

[facebook-github-bot]

💊 CI failures summary and remediations

As of commit 2b67bfa (more details on the Dr. CI page):

---

💚 💚 Looks good so far! There are no failures yet. 💚 💚

---

This comment was automatically generated by Dr. CI (expand for details).

Please report bugs/suggestions to the (internal) Dr. CI Users group.

Click here to manually regenerate this comment.

[sugatoray]

@datumbox I am not sure if the following is the right place to add this code and if this should go under torchvision.utils. I have not added any tests yet. Once it is determined where this code should be added to, I will work on adding the tests as well.

Also, I will post a correction/update to the RAFT demo with these functions.

current path: torchvision/models/optical_flow/detector.py

[datumbox]

@sugatoray First of all thanks for the contribution.

It's not clear if this should be handled in the references of TorchVision or if this should be a separate utility added on your own repo. That's because we try to keep references as simple as possible and avoid multiple entry points. I will let @NicolasHug who has led the effort of supporting optical flow to provide input about whether specific optimisations of this PR are worth adopting on the current scripts.

A couple of notes that will help you maximise the chances of getting your contributions merged: If a PR touches more than 20 lines of code, it's usually recommended to first open an issue and discuss the changes. Discussing the reported issue with the rest of the community can help us decide if it is indeed an problem and how it should be addressed. For more info on the contribution process please check this

[sugatoray]

@datumbox Thank you. I will keep the "20 lines" in mind for future. Out of the two functions, detect_flow helps detect a single flow-pair. If this function is available, that would work as a helpful utility for people to use it directly, alike other functions in torchvision.utils.

Also, for the demo notebook, I could push a separate PR, to include similar changes, but without the specific function in question (detect_flow). Please let me know, if I should.

[oke-aditya]

Thanks for pointing out the Memory issue! @sugatoray

Hi! @NicolasHug will have a deeper look.

I think that we do lot of implicit assumptions. Such as default model, releasing memory (it might not be needed). Folder name, saving etc.

Maybe adding a note to the gallery example suggesting how CUDA Out of Memory error could be handled could help?

[sugatoray]

@oke-aditya About the the assumptions on folder name, saving etc., it was necessary to save the files to disk. Now you may argue (as I debated with myself) that it should be perfectly fine returning just the flow_img and predicted_flow and let the user handle saving the files to disk. However, given the memory constraints, it might be useful, if you could just conveniently specify whether you want to save the raw and processed flow data to disk, or you want them returned as well.

Given, you have this function, detect_flow it all boils down to a single function call for generating optical flows. On the other hand, detect_flows handles the same for multiple images, which you can now create a video/gif file from.

So, yes, I had envisioned them as parts of utils/. But, just to begin with, pushed the initial commit to torchvision/models/optical_flow/.

[NicolasHug]

Thanks a lot for the PR @sugatoray . As @datumbox already noted, this kind of util is usually not included in the models/ namespace; it's more likely to fit in the utils/. But here, it seems that this function is somewhat redundant with our RAFT example.

When I tried running the RAFT demo to create a series of optical-flow images, I found the colab notebook running out of memory

Could you please provide us with some details here? If the example runs out of memory, perhaps there's an issue that we can fix directly there :)

[sugatoray]

@NicolasHug Yes, it ran out of memory. This is why I used gc.collect() and torch.cuda.empty_cache(). I could send a separate PR only changing the minimum, in the demo notebook to make it run without error, if that helps.

[sugatoray]

@datumbox So, will it be okay if I move the functions detect_flow and detect_flows under utils/?

[NicolasHug]

Yes, it ran out of memory

@sugatoray Would you mind providing the exact code that caused the memory to run out? If possible, do you still have the error trace? I'm fairly surprised that our example causes a OOM issue, because we're only doing 2 predictions.

[sugatoray]

@NicolasHug The memory limit was struck when I tried running the last section to create multiple frames in a loop. Also the for loop had some obvious typos.

[NicolasHug]

Thanks for the details @sugatoray . We'll be happy to review a PR that fixes these issues in the example. As noted there, this snippet that we commented out is only a starting point, not a full fledge solution.

[sugatoray]

@NicolasHug I will send a PR with the fixes to the snippet section.

[datumbox]

@sugatoray Shall we close this PR? I understand we are going ahead with a different approach, right?

[sugatoray]

@datumbox Would it be possible to leave this PR to remain open for the time being, while I work on the other PR and submit it?

But, I was wondering if these functions could be added to utils. Please let me know your thoughts on that. Also, please let me know if you believe some changes to the functions could be a better option.

Thank you.

[datumbox]

@sugatoray Happy to leave the PR open if you prefer. Please consider closing it once you are happy with the other PR to keep things tidy.

From @NicolasHug's response, I understand that this is not something we probably want on the utils of references (this is something that people can't import from TorchVision anyway because references are not part of the binary). Nicolas, is my understanding wrong?

[NicolasHug]

I'm not sure yet whether this would make sense as a util in the references either. It will depend on what are the needed changes to the snippet from the example, and why the changes are needed. It's important to keep in mind that the snippet is really a starting point, it doesn't pretend to be anything else, and there might be blind spots. The typos mentioned by @sugatoray are definitely worth fixing though.

[sugatoray]

@datumbox and @NicolasHug Let me send the PR to introduce minimal changes to the RAFT example. Once it is working and the example is updated, perhaps you can take a more objective call on whether the functions in this PR are appropriate for torchvision.utils or not.

• utils: torchvision/utils.py

Does that sound reasonable to you?

[datumbox]

Sounds good but this is definitely something we can't put on torchvision.utils. This area is not meant for model inference utilities. Let's get clarity over what the changes are and decide if there is any need to modify the examples.