-
Notifications
You must be signed in to change notification settings - Fork 20
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
[WIP]Add skeletonization function #13
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I left some first-round comments here, it's just something I noticed, and I think it's more appropriate to let others review your code, they're more experienced and more qualified.
I didn't read the MedialAxis Skeletonization algorithm before, so I trust that you've written the algorithm procedure correct. Or could you give a reference to your implementation, so that I can have a look at? (I think it's quite an easy algorithm)
I haven't' reviewed others' codes before, so if there's anything I did not properly, please point it out :)
@@ -3,10 +3,15 @@ __precompile__() | |||
module ImageMorphology | |||
|
|||
using ImageCore | |||
using Images |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's using ImageMorphology
in Images.jl, and it will make package dependency complicate.
@timholy How do we deal with this problem in general? Can we directly copy Images.FeatureTransform
module to ImageMorphology
as temporary not-exported functions?
https://github.com/JuliaImages/Images.jl/blob/bdfd044420fa6ffcd34760f804f0c3ce12186945/src/bwdist.jl
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@johnnychen94 another example where we find something implemented in Images.jl that could live somewhere else. We really need to reorganize concepts around and break Images.jl into smaller packages for reuse. Right now a bunch of functionality lives in the umbrella package, and that is suboptimal.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like @Deepank308 needs this function implemented and merged ASAP to continue his GSoC ImageTracking project.
@juliohm I guess we can copy these methods to FeatureTransform.jl
(with comments) and remove it in the future, just like @zygmuntszpak does in https://github.com/zygmuntszpak/ImageBinarization.jl/blob/6e0c81867eaef67b463fa5d52febfca4d0f6196a/src/integral_image.jl ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know if copying/pasting helps @johnnychen94 , ideally we would start thinking more seriously about how to reorganize things. There is a lot of code living in the wrong place and we are just compromising the situation further by adopting multiple copies in submodules.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I might need another two weeks to start the porting work. Let's see how it works then.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@johnnychen94 I don't need this function as of now in my implementations. I was just going through Skeletonization algorithm and found that Julia doesn't have it. So, I implemented and sent a PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this case, this PR might be pended until FeatureTransform
being ported to a standalone module.
Codecov Report
@@ Coverage Diff @@
## master #13 +/- ##
===========================================
- Coverage 100% 70.75% -29.25%
===========================================
Files 2 4 +2
Lines 44 106 +62
===========================================
+ Hits 44 75 +31
- Misses 0 31 +31
Continue to review full report at Codecov.
|
Codecov Report
@@ Coverage Diff @@
## master #13 +/- ##
===========================================
- Coverage 100% 71.15% -28.85%
===========================================
Files 3 4 +1
Lines 62 104 +42
===========================================
+ Hits 62 74 +12
- Misses 0 30 +30
Continue to review full report at Codecov.
|
``` | ||
|
||
# References | ||
[1] http://homepages.inf.ed.ac.uk/rbf/HIPR2/skeleton.htm |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add a title to the reference item for clarity
also here are some thoughts on simplifying documentation of function with multiple methods.
JuliaImages/Images.jl#790 (comment)
JuliaImages/ImageBinarization.jl#25
It'll be great to hear thoughts from you.
Further documentation work can be done after codes being stable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Noted. I will keep that in my mind if any other skeletonization algorithm will be implemented in future?
I don't know the exact reason as to why AppVeyor test is failing. The test fails for |
An upgrade on CI configuration is needed, #14 As for |
I was curious to compare the performance of the algorithm in Julia and Python(skimage). This is what I got :
So, there is some serious need for performance improvements!! Please suggest if anyone has some ideas. |
according to my experiences:
not sure if any of it works, sorry that I don't have much time digging into the details at present. There might be more tips in performance tweaking. |
I wrote a simple grassfire transform algorithm which works also on non-binary images: https://github.com/fweth/Julia/blob/master/grassfireTransform.jl I experimented with rasterized vector graphics and found out that treating pixels with values < 1 as already displaying the 'true' distance to the border gives smoother results than first applying threshold to the rasterization. |
Added skeletonization algorithm using medial-axis transform for binary images.
I will add documentation soon.
Please give suggestions for improving the API and performance, if any.
Ping @juliohm
Thank you!