The Wayback Machine - https://web.archive.org/web/20201010185753/https://github.com/scikit-image/scikit-image/pull/4847
Skip to content

/ scikit-image

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

Jump to bottom

[WIP] Adding @eldad-a's ridge directed ring detector #4847

Open
wants to merge 13 commits into
base: master
from
Open

[WIP] Adding @eldad-a's ridge directed ring detector #4847

wants to merge 13 commits into from

Conversation

@alexdesiqueira
Copy link
Member

@alexdesiqueira alexdesiqueira commented Jul 12, 2020

Description

Continues #1706, by @eldad-a:

an implementation of the algorithm in
Sci. Rep. 5, 13584; doi: 10.1038/srep13584 (2015).

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.
eldad-a and others added 5 commits Sep 12, 2015
@eldad-a @stefanv
added ridge-directed-ring-detector + demo notebook
1b5c89a 
an implementation of the algorithm in
Sci. Rep. 5, 13584; doi: 10.1038/srep13584 (2015).

EA
 On branch ridge-directed-ring-detector
 Changes to be committed:
	new file:   ridge-directed-ring-detector/demo.ipynb
	new file:   ridge-directed-ring-detector/ridge_directed_ring_detector.pyx
@stefanv
Add ring detector to build system
4bc19b5
@eldad-a @stefanv
update ring detector
Loading status checks…
f03bf60 
            1. Correct outer image frame error (`IndexError`)
            2. Commented out `MaxRads`, a variable not being used in the code so far
            3. Introduced a `rate` variable for future modification
               (growth rate of `ksigma` with the radius `R`)
@eldad-a
added data file for the ring_detector demo notebook and modified acc.
Loading status checks…
c1c7424 
 On branch ridge-directed-ring-detector
 Your branch is up to date with 'origin/ridge-directed-ring-detector'.

 Changes to be committed:
	modified:   ridge-directed-ring-detector/demo.ipynb
	new file:   ridge-directed-ring-detector/unprocessed_fig_46.png
@alexdesiqueira
[WIP] Adding ring_detector.py and its prelminary structure.
Loading status checks…
ac6e68f
@pep8speaks
Copy link

@pep8speaks pep8speaks commented Jul 12, 2020 •
edited

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

Line 41:57: W605 invalid escape sequence '*'
Line 41:63: W605 invalid escape sequence '*'
Line 68:80: E501 line too long (143 > 79 characters)
Line 75:80: E501 line too long (153 > 79 characters)

Line 12:80: E501 line too long (137 > 79 characters)

Comment last updated at 2020-07-26 19:33:12 UTC
@alexdesiqueira alexdesiqueira mentioned this pull request Jul 12, 2020
Closed
@alexdesiqueira
Copy link
Member Author

@alexdesiqueira alexdesiqueira commented Jul 12, 2020

The following is based on the discussion I had with @clementkng on Slack during the SciPy 2020 sprint, and what I think we need to pursue for this PR.

What we have:

skimage/transform/ridge-directed-ring-detector/demo.ipynb
skimage/transform/ridge-directed-ring-detector/unprocessed_fig_46.png
skimage/transform/_ring_detector.pyx

I'd like to:
[OK] 1. create a function within transform — let's say ring_detector.py — that would have the API necessary to call ring_detector.pyx. Something on the line of skimage/transform/hough_transform.py or skimage/transform/radon_transform.py;
2. move demo.ipynb to doc/examples. The notebook would be converted to a .py file — as the other files in our documentation. It'd go to doc/examples/transform/. A possible name would be plot_ring_detector.py.
3. remove the ridge-directed-ring-detector folder.
4. replace OpenCV functions on _ring_detector.pyx with skimage ones.
@clementkng
DOC: Refactor ridge directed ring detector example
Loading status checks…
1e66273
@eldad-a
Copy link

@eldad-a eldad-a commented Jul 13, 2020 •
edited

Hello @alexdesiqueira

Many thanks for moving forward with this PR, and sorry for my losing track of the process.

I should have a draft which is opencv independent - where all spatial derivatives are performed by skimage functions.

If that's helpful, please let me know where to push it to.

Thanking you,
Eldad
@alexdesiqueira alexdesiqueira mentioned this pull request Jul 14, 2020
Open
@alexdesiqueira
Copy link
Member Author

@alexdesiqueira alexdesiqueira commented Jul 14, 2020

Hey @eldad-a ,
don't worry at all — it happens to all of us 😄

I should have a draft which is opencv independent - where all spatial derivatives are performed by skimage functions.

If that's helpful, please let me know where to push it to.

That would be great, thank you! I will add you as a contributor, please feel free to push it here when you can.
@mkcor
mkcor reviewed Jul 17, 2020
View changes
doc/examples/transform/plot_ring_detector.py Outdated

from skimage.transform import ring_detector
from skimage import data
from scipy import ndimage

This comment has been minimized.

Sign in to view
@mkcor

mkcor Jul 17, 2020
Contributor

import ndimage before skimage

doc/examples/transform/plot_ring_detector.py Outdated
using 2d microscope imaging.
Here's a quick description of the parameters. The explanations refer to the
terms as introduced in [Afik 2015](https://www.nature.com/articles/srep13584)

This comment has been minimized.

Sign in to view
@mkcor

mkcor Jul 17, 2020
Contributor

Looks like you're using Markdown syntax. In my experience, it should be reStructuredText.

This comment has been minimized.

Sign in to view
@emmanuelle

emmanuelle Jul 19, 2020
Member

indeed :-)

This comment has been minimized.

Sign in to view
@emmanuelle

emmanuelle Jul 19, 2020
Member

you can take a look at existing gallery scripts for examples of rst syntax.

This comment has been minimized.

Sign in to view
@alexdesiqueira

alexdesiqueira Jul 19, 2020
Author Member

Sorry about that @mkcor @emmanuelle, but this is too raw for a review yet. 😄 I asked @clementkng to move the code from a notebook to this file, starting the documentation process. We'll take care of it 🙂

skimage/transform/ring_detector.py Outdated
@@ -0,0 +1,30 @@
from ._ring_detector import RidgeHoughTransform

import matplotlib.pyplot as plt

This comment has been minimized.

Sign in to view
@mkcor

mkcor Jul 17, 2020
Contributor

Please swap import order.
Reference: https://www.python.org/dev/peps/pep-0008/#imports

doc/examples/transform/plot_ring_detector.py Outdated Show resolved Hide resolved
doc/examples/transform/plot_ring_detector.py Outdated Show resolved Hide resolved
alexdesiqueira and others added 4 commits Jul 17, 2020
@alexdesiqueira @mkcor
Update doc/examples/transform/plot_ring_detector.py
Verified
This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.
GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits
Loading status checks…
b055cd5 
Co-authored-by: Marianne Corvellec <[email protected]>
@alexdesiqueira @mkcor
Update doc/examples/transform/plot_ring_detector.py
Verified
This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.
GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits
Loading status checks…
8a83869 
Co-authored-by: Marianne Corvellec <[email protected]>
@alexdesiqueira
Fixing according to @mkcor suggestion
a60c9e9
@alexdesiqueira
Merge branch 'ring_detector' of https://github.com/alexdesiqueira/sci…
Loading status checks…
ab5a60a 
…kit-image into ring_detector
@alexdesiqueira
Copy link
Member Author

@alexdesiqueira alexdesiqueira commented Jul 17, 2020

Thank you @mkcor! This is still very raw, not ready for a review, though.
Could we ping you when this is somewhat acceptable? 😅 Thank you again!
@mkcor
Copy link
Contributor

@mkcor mkcor commented Jul 20, 2020

@alexdesiqueira sure, no worries! Your PR title says 'WIP' indeed :)
eldad-a added 2 commits Jul 24, 2020
@eldad-a
removed opencv dependencies from ring_detector
7155179 
1. Replace OpenCV Hessian estimation by skimage ones

2. Comment-out 'eccentricity', which relies on OpenCV for EllipseFit

3. Rename, following skimage conventions:
Lxx -> Lrr , Lyy -> Lcc , Lxy -> Lrc

3. Add performance observations and TODO to consider
@eldad-a
adjust 'curv_thresh' following the transition from OpenCV to skimage
Loading status checks…
341dc42
@eldad-a
Copy link

@eldad-a eldad-a commented Jul 24, 2020 •
edited

The following is based on the discussion I had with @clementkng on Slack during the SciPy 2020 sprint, and what I think we need to pursue for this PR.

What we have:
...
4. replace OpenCV functions on _ring_detector.pyx with skimage ones.

Thank you all for your patience with me!
Hope everyone is doing well during these different times.

  1. OpenCV dependency removed; currently replaced by skimage.feature.hessian_matrix.
  2. Updated curve_thresh parameter in the example plot_ring_detector.py accordingly.

Noticed that ring_detector.py returns the rings rather than the subpixel rings.
Where should the alternative be highlighted for users?
I would support having the subpixel rings as the default.

I've been exploring the outcome on the use-case for which this code was developed originally (an example was provided in ridge-directed-ring-detector/unprocessed_fig_46.png ; BTW, was this file removed?).
Included some observations and suggestions in the _ring_detector.pyx itself.
Happy to share the ipynb where I explored using scharr and farid; in case of interest, please let me know where to put them.
For convenience, I include the notes below as well; would be great to hear your take on these!

EA NOTES

  • The original opencv version shows better performance compared to the current skimage:
    • preprocessing takes x3 longer using hessian_matrix (from skimage), and
    • ring_detection takes about x1.2, showing higher sensitivity to choice of parameter (sigma and curv_thresh)
  • The results seem more precise and less sensitive to choice of parameters when derivatives are estimated using the filters.farid_v/h (or filters.scharr_v/h); however, these operators take x6 (x3) times longer to compute, w.r.t to hessian_matrix, which uses np.gradient; this could probably be improved by programming the more precise (but slower) operators to allow the 2nd order derivatives directly.

EA TO-Consider

  • store both principal curvatures and include verifying that |Lpp|>|Lqq| ; here Lpp denotes the least principal curvature (Lqq the largest principal curvature).
  • allow the user to choose between np.gradient (skimage hessian_matrix), scharr, and farid operators; the latter seem to offer more precise results, with less sensitivity to the choice of parameters. The former is faster.
  • add automated choice of curv_thresh based on percentiles.
  • verify results and efficiency are not significantly affected the transition from opencv to skimage (Hessian calculation)
  • Prepare PR for directed ridge detector ? There are several related algorithms in skimage; does any of them use non-maximum suppression in the direction of the eigen-vector associated with the least-principal curvature? (as performed in the ring_detection ). Explore the effect of separating the ridge detection function entirely; it may not affect the %%timeit results.
@alexdesiqueira
Copy link
Member Author

@alexdesiqueira alexdesiqueira commented Jul 24, 2020

Hey @eldad-a,

  1. OpenCV dependency removed; currently replaced by skimage.feature.hessian_matrix.
  2. Updated curve_thresh parameter in the example plot_ring_detector.py accordingly.

Thank you very much, we appreciate your help!

Noticed that ring_detector.py returns the rings rather than the subpixel rings.
Where should the alternative be highlighted for users?
I would support having the subpixel rings as the default.

I'd ask your help to determine that technical features, Eldad. I still don't understand the algorithm that much, so please feel free to check what you consider doable on that subject — I would take a time to understand 🙂

I've been exploring the outcome on the use-case for which this code was developed originally (an example was provided in ridge-directed-ring-detector/unprocessed_fig_46.png ; BTW, was this file removed?).

I'd like to structure the example on the lines of the examples we have already — hence doc/examples/transform/plot_ring_detector.py. Would you like to base the example on that specific file? If yes, I'd suggest to check the license for that file. If it's permissible, we could add it to data and start the example from there; what do you think?

  • allow the user to choose between np.gradient (skimage hessian_matrix), scharr, and farid operators; the latter seem to offer more precise results, with less sensitivity to the choice of parameters. The former is faster.

I like this! I'd need to understand the other points better, though; what would be their implications, you know... I need to read your paper again 🙂
Thanks again!
@clementkng
DOC: Fix import order and convert Markdown to RST
Loading status checks…
b3814ac
@eldad-a
Copy link

@eldad-a eldad-a commented Aug 17, 2020 •
edited

Hi @alexdesiqueira :-)

  1. OpenCV dependency removed; currently replaced by skimage.feature.hessian_matrix.
  2. Updated curve_thresh parameter in the example plot_ring_detector.py accordingly.

Thank you very much, we appreciate your help!

It is my pleasure;
I thank you for your support and encouragement!

Noticed that ring_detector.py returns the rings rather than the subpixel rings.
Where should the alternative be highlighted for users?
I would support having the subpixel rings as the default.

I'd ask your help to determine that technical features, Eldad. I still don't understand the algorithm that much, so please feel free to check what you consider doable on that subject — I would take a time to understand slightly_smiling_face

Sure, I will.

I've been exploring the outcome on the use-case for which this code was developed originally (an example was provided in ridge-directed-ring-detector/unprocessed_fig_46.png ; BTW, was this file removed?).

I'd like to structure the example on the lines of the examples we have already — hence doc/examples/transform/plot_ring_detector.py. Would you like to base the example on that specific file? If yes, I'd suggest to check the license for that file. If it's permissible, we could add it to data and start the example from there; what do you think?

Which data image to use... Why not both?
#3384 calls for scientific examples, so this is an opportunity.

My understanding of licensing goes a very short distance (well, maybe none...).
Here's what the original publication reads in terms of license: Rights and permissions.
The figure is part of Supplementary Figure S2(a)
in Afik, E. Robust and highly performant ring detection algorithm for 3d particle tracking using 2d microscope imaging. Sci. Rep. 5, 13584; doi: 10.1038/srep13584 (2015).

  • allow the user to choose between np.gradient (skimage hessian_matrix), scharr, and farid operators; the latter seem to offer more precise results, with less sensitivity to the choice of parameters. The former is faster.

I like this! I'd need to understand the other points better, though; what would be their implications, you know... I need to read your paper again

If you have questions, please let me know; may be easier than going over the paper.

Below there's a TODO list I created for my future reference; some are added features, so I guess aren't a must.
Could you share what are the priorities to have this PR ready?

EA TODO:

  • ring_detector.py : return subpixel precision ring parameters
  • allow the user to choose between np.gradient (skimage hessian_matrix), scharr, and farid operators; the latter seem to offer more precise results, with less sensitivity to the choice of parameters. The former is faster.
  • test: store both principal curvatures and include verifying that |Lpp|>|Lqq| ; here Lpp denotes the least principal curvature (Lqq the largest principal curvature).
  • add automated choice of curv_thresh based on percentiles.
  • verify results and efficiency are not significantly affected the transition from opencv to skimage (Hessian calculation)

In case skimage community has interest:

  • Prepare PR for directed ridge detector ? There are several related algorithms in skimage; does any of them use non-maximum suppression in the direction of the eigen-vector associated with the least-principal curvature? (as performed in the ring_detection ).
    • Explore the effect of separating the ridge detection function entirely; it may not affect the %%timeit results.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@emmanuelle emmanuelle

@mkcor mkcor

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
action: work in progress type: new feature
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

None yet

7 participants
@alexdesiqueira @pep8speaks @eldad-a @mkcor @emmanuelle @stefanv @clementkng
You can’t perform that action at this time.