Skip to content
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 the iterative Lucas-Kanade (iLK) optical flow method #4161

Merged
merged 48 commits into from Oct 2, 2020

Conversation

rfezzani
Copy link
Member

@rfezzani rfezzani commented Sep 13, 2019

Description

Here is an implementation of the iterative Lucas-Kanade (iLK) method for optical flow estimation [1].
The Lucas-Kanade method is a local approach for the estimation of optical flow (as opposed to global approaches such TV-L1 for example). It is fast, robust and requires basically only one parameter that is the size of the neighborhood considered for the estimation.

[1]: Le Besnerais, G., & Champagnat, F. (2005, September). Dense optical flow by iterative local window registration. In IEEE International Conference on Image Processing 2005 (Vol. 1, pp. I-137). IEEE.

Checklist

For reviewers

  • Check that the PR title is short, concise, and will make sense 1 year
    later.
  • Check that new functions are imported in corresponding __init__.py.
  • Check that new features, API changes, and deprecations are mentioned in
    doc/release/release_dev.rst.
  • Consider backporting the PR with @meeseeksdev backport to v0.14.x

@pep8speaks
Copy link

pep8speaks commented Sep 13, 2019

Hello @rfezzani! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

Line 114:14: E226 missing whitespace around arithmetic operator
Line 114:24: E226 missing whitespace around arithmetic operator

Comment last updated at 2020-10-02 08:50:49 UTC

@jni
Copy link
Member

jni commented Sep 15, 2019

Ha! I didn't realise the question of how to present multiple optical flow algorithms in the API would become so relevant so soon! 😂 Thanks @rfezzani! I also notice that it's nD out of the box, a very nice touch. ;)

I don't have time for a deep review right this second but it looks pretty great on first glance!

@rfezzani
Copy link
Member Author

😂 Thank you @jni, the code was almost done in pyimof, I just had to implement the nD support ;-) I hope you like it.
I had a look on Travis CI, but I don't understand what makes it Fail :-(

@lagru
Copy link
Member

lagru commented Sep 25, 2019

I had a look on Travis CI, but I don't understand what makes it Fail :-(

Both failures are due to optical_flow_ilk returning float64 when float32 is expected. Does that help or did you mean you don't understand why that function returns the wrong dtype?

# Estimate the flow at double precision
flow_f64 = optical_flow_ilk(image0, image1, dtype='float64')

assert flow_f64.dtype == 'float64'
Copy link
Member

Choose a reason for hiding this comment

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

First time seeing that way to check dtypes. Seems a bit brittle and doesn't seem to work for cases such as np.float64 == "float64" which will return False. So maybe use np.float64 here (and similar at other places) instead?

Copy link
Member Author

Choose a reason for hiding this comment

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

You are right, np.float64 == "float64" returns False, but np.dtype(np.float64) == 'float64' is True. Here I compare data type object...
But I will check if this can be done differently ;-)

Copy link
Member

Choose a reason for hiding this comment

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

flow.dtype == np.float64 should work?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, absolutely :-)

skimage/registration/tests/test_ilk.py Outdated Show resolved Hide resolved
skimage/registration/tests/test_ilk.py Outdated Show resolved Hide resolved
rfezzani and others added 2 commits September 26, 2019 09:25
Co-Authored-By: Lars Grueter <lagru@users.noreply.github.com>
Co-Authored-By: Lars Grueter <lagru@users.noreply.github.com>
@rfezzani
Copy link
Member Author

rfezzani commented Sep 26, 2019

I had a look on Travis CI, but I don't understand what makes it Fail :-(

Both failures are due to optical_flow_ilk returning float64 when float32 is expected. Does that help or did you mean you don't understand why that function returns the wrong dtype?

I actually don't understand why that function returns the wrong dtype 😄. Since the tests pass in the other CI checks...

@rfezzani
Copy link
Member Author

OK, I finally reproduced the CI failure: It comes from numpy version 1.13.3!

@rfezzani
Copy link
Member Author

The CI failure is related to the issue #4189.

@rfezzani
Copy link
Member Author

Thank you very much @grlee77 for your review 😉

@rfezzani
Copy link
Member Author

Thank you again @grlee77 for your excellent review, I think I addressed all your requests now 😉.

Copy link
Contributor

@grlee77 grlee77 left a comment

Choose a reason for hiding this comment

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

Thanks @rfezzani for addressing the comments. This seems good to go.

Registration and warping automation moved this from Under Review to Ready to Merge Sep 25, 2020
Copy link
Member

@emmanuelle emmanuelle left a comment

Choose a reason for hiding this comment

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

just a few comments as I was reading

doc/examples/registration/plot_opticalflow.py Outdated Show resolved Hide resolved
doc/examples/registration/plot_opticalflow.py Outdated Show resolved Hide resolved
rfezzani and others added 2 commits September 28, 2020 00:05
Co-authored-by: Emmanuelle Gouillart <emma@plot.ly>
Co-authored-by: Emmanuelle Gouillart <emma@plot.ly>
@emmanuelle
Copy link
Member

@rfezzani one thing which I've been missing either in docstrings or in the gallery example (maybe both) is when to use which optical flow algorithm. From what I understand, one should use TV-l1 to allow for discontinuities in the flow, but when the flow is smooth one should prefer ilk which is faster. Is this correct? Could you please add a few explanations on how to select an optical flow algorithm? Algorithm selection is maybe as hard as parameter selection for our users :-).

@emmanuelle
Copy link
Member

@rfezzani thank you very much for the PR! I have finished reading through the Plyer et al. paper, and the PR is ready to go for me. The only caveat would be to add a few explanations about when to use which algorithm, please tell us if you have the time to add it now or not. But there should not be other comments from me!

@rfezzani
Copy link
Member Author

Sorry @emmanuelle, I didn't found time yesterday :/ I will try to manage adding notes in the doc today.

@emmanuelle
Copy link
Member

Thanks @rfezzani :-). Still in time for the release, don't worry.

@rfezzani
Copy link
Member Author

rfezzani commented Oct 2, 2020

@emmanuelle, I modified optical_flow_ilk docstring's long description:

The iterative Lucas-Kanade (iLK) solver is applied at each level
of the image pyramid. iLK [1]_ is a fast and robust alternative to
TVL1 algorithm although less accurate for rendering flat surfaces
and object boundaries (see [2]_).

Is it enough or should I elaborate more?

@emmanuelle
Copy link
Member

Thanks @rfezzani . Let's wait for CI and then I'll merge!

@rfezzani
Copy link
Member Author

rfezzani commented Oct 2, 2020

I think CI failure is not related...

@emmanuelle emmanuelle merged commit 769a555 into scikit-image:master Oct 2, 2020
Registration and warping automation moved this from Ready to Merge to Done Oct 2, 2020
@emmanuelle
Copy link
Member

Merged :-) !

@rfezzani
Copy link
Member Author

rfezzani commented Oct 2, 2020

🎉 Thank you @emmanuelle !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

Successfully merging this pull request may close these issues.

None yet

8 participants