added ridge-directed-ring-detector + demo notebook #1706
Conversation
|
This is my first PR -- guess it would need some work -- looking forward to your suggestions. |
|
If this does not fit into skimage's scope, I think this would fit in https://github.com/soft-matter/trackpy |
|
I encouraged Eldad to make this pull request, after reviewing his paper. Having looked at several algorithms in the field, I am convinced that this extremely useful. We'll need to do some work to get this into shape. Add examples to the gallery, clean up the code, add correct Python wrappers & documentation, etc. But I think it is well worth the effort! Thomas, thank you for expressing interest. Your input would be much appreciated! |
|
@stefanv Ah, great, sorry for causing trouble. I agree it is useful and want to make sure this code has a home! |
|
@tacaswell Not at all; we are just too happy to have you involved. |
|
Thank you both for your words of support! |
|
OK, I had a first look. We need to do a few things here:
What do you think, Eldad? |
|
@eldad-a What's the status of this PR? |
|
Hi @eldad-a, we are interested in adding this code to the next major version. |
|
It's likely that @eldad-a doesn't want to work on this anymore after such a long time. Very interesting feature, but there is quite a lot of work to integrate it in scikit-image. In particular, to reuse build-in fonctions to fit and perform hough transforms. |
|
I spoke to @alexandrejaguar about this, and encouraged him to investigate the contribution. The work @eldad-a did was of high quality, and I reviewed his manuscript that appeared in Nature Methods. This is a method that adds unique functionality that does not currently exist in any other library, but the method is well thought through. I think we would greatly benefit from having it, and so it is worth some extra effort. @eldad-a may or may not have time to work on this right now, but he has been responsive in the past (he and I have had long email exchanges about the work). |
|
@alexandrejaguar Thanks for following up and for the interest! @stefanv Thank you again for the kind words and support! BTW, Nature Methods requires "tried-and-tested basic research techniques in the life sciences", which was beyond the scope of the work at the time (that is, to try and test the technique in a biological research example). The manuscript can be found at E. Afik, Robust and highly performant ring detection algorithm for 3d particle tracking using 2d microscope imaging. Sci. Rep. 5, 13584; doi: 10.1038/srep13584 (2015) |
|
Apologies: I mixed up Scientific Reports and Nature Methods—but the effect is the same. |
|
@eldad-a Is this version in sync with the latest in your repo at https://github.com/eldad-a/ridge-directed-ring-detector? |
@stefanv No need to apologize, of course :-) |
@stefanv Does this version refers to the one in my first PR? |
|
@eldad-a To make sure I understand correctly: we need to grab those two changes from your ridge-directed-ring-detector repository? Thanks for chiming in; I'm so excited that we're getting the wheels rolling on this awesome feature again. |
|
@stefanv Yes, I think it is better that the version we'll be working is the most recent one (it introduces no conceptual changes). Thanks again for the continuous interest and support, I greatly appreciate it! |
|
On Thu, 23 May 2019 11:30:37 -0700, Alexandre de Siqueira wrote:
@eldad-a thanks for helping! Could you push these improvements to this pull request?
> Use the scikit-image build system to build the Cython files (I've sent you a pull request)
I think we could start adding the pull request @stefanv sent you before. How's that?
Alex, if Eldad doesn't mind, I think we should just work on his branch and
push the changes up. This would be the fastest way to move forward.
|
@alexandrejaguar I believe I just completed this step.
Could you please direct me what's the best way to do so? |
|
Hey @eldad-a, thank you for your help!
This was solved when you pushed @stefanv PR already, thanks for doing that.
This could be good: please use
I think now we could address both of these issues at once. What do you think? |
|
@alexandrejaguar wrote:
checked out Should I sync my fork to the original scikit-image repository? |
|
@eldad-a wrote:
Feel free to do that as well ( |
|
@alexandrejaguar
Sorry for the misunderstanding; did not realize it was not desired to have my branch updated to Should I revert? Should I run |
@eldad-a don't worry, I learned the worst way everything is reversible here. :) |
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
|
Please note that the data file used in the notebook is missing from the archive. |
|
I've pushed the rebased branch. |
@stefanv Thanks! If possible, please share the command you used. |
|
Dear @alexandrejaguar @stefanv , I looked at the (skimage) How to contribute to page and followed the guidelines for conda, for setting up the build environment.
Tracing back to what caused the switch from my This happens even if I skip that command and try: Any suggestions on this? For now I re-created the environment, skipped the lines that switch to Found that |
|
:/ I can't seem to recreate on my own computer seems ok on my computer. |
|
do you have any other channels added to your system? is conda up to date for you? |
|
I think if your base conda environment is 2.7, your default Python is 2.7. Probably we should add |
|
That I never knew |
Thanks @hmaarrfk , @jni ! Apparently the Removing the I apologize for the false alarm. pytest results in 1 error
|
|
@eldad-a I used |
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
Thanks for the note! |
|
@stefanv Do you have a chance to look at this PR. Would be good to take a decision for the next release. |
|
Hey @eldad-a, long time no talk :) Are these renamed/removed ones? |
|
Hey everyone, |
|
That'd be great, thanks Alex. |
|
I'm trying to push to this branch ( |
Maybe that branch is not set to allow edits from maintainers? If you can't get it to work, you can make a new PR after creating a new branch from this one. That will still preserve the commit history. |
|
I'm still having my PRs denied: Not sure what's happening. How would I do this by hand, again? |
|
Closing due to permission issues. Please refer to #4847, which continues this PR. |
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