add imfill function #9
Conversation
Codecov Report
@@ Coverage Diff @@
## master #9 +/- ##
==========================================
+ Coverage 91.76% 92.88% +1.11%
==========================================
Files 5 6 +1
Lines 243 253 +10
==========================================
+ Hits 223 235 +12
+ Misses 20 18 -2
Continue to review full report at Codecov.
|
Codecov Report
@@ Coverage Diff @@
## master #9 +/- ##
===========================================
- Coverage 100% 55.69% -44.31%
===========================================
Files 2 3 +1
Lines 44 79 +35
===========================================
Hits 44 44
- Misses 0 35 +35
Continue to review full report at Codecov.
|
aquatiko
force-pushed
the
imfill
branch
2 times, most recently
from
a387b33
to
976089b
Compare
|
@timholy @juliohm I noticed in JuliaImages doc that the function |
|
I think the code is ready 👌 We only need to clarify in the docstring the 1s vs. 0s convention we are adopting. @timholy do you have additional suggestions? @aquatiko regarding the other contributions you have in mind, I suggest opening a new issue on Images.jl pointing to the references and/or similar software that implements the functions you mentioned. Feel free to ping me there as well. :) |
|
@aquatiko after you fix the docstring as suggested, could you also add a few tests for this new function? |
|
@juliohm I have made the asked changes and added some tests. But the tests seems to fail, although they are working fine locally. |
There was a problem hiding this comment.
@aquatiko thank you for incorporating the suggestions. For some reason, GitHub is telling me that the commit you made with tests is not part of the PR. Could you please double check that you are following the standard process of PR updates?
Also, if you can add a test for corner cases with images that are full of 1s and full of 0s, it is always good to have these tested.
src/imfill.jl
Outdated
| function imfill(img::AbstractArray{Bool}, interval::Tuple{Real,Real}, connectivity::Union{Dims, AbstractVector{Int}, BitArray}=1:ndims(img)) | ||
|
|
||
| if interval[1] > interval[2] || interval[1] < 0 || interval[2] < 0 | ||
| print("Invalid Interval") |
There was a problem hiding this comment.
It is idiomatic to throw an error instead. Could you please use @assert or a more specific error type in Julia? https://docs.julialang.org/en/v1.0/manual/control-flow/#Exception-Handling-1
There was a problem hiding this comment.
@juliohm I have added some more tests. But they seem to be failing as: imfill uses label_components from package Images and it is not included during testing.
ERROR: LoadError: ArgumentError: Package Images not found in current path:
- Run `Pkg.add("Images")` to install the Images package.
Can u guide me how to fix this?
There was a problem hiding this comment.
Good question @aquatiko. In cases like this, we would either migrate label_components to somewhere else in the Images.jl project, or migrate imfill. I personally like to think of Images.jl as a metapackage that loads the actual submodules of image processing and analysis. In that case, I defend the point that we should migrate label_components to somewhere else. What is your opinion on this @timholy? Do you have a suggestion on where we could migrate things so that the project is better structured?
@aquatiko every now and then this happens, and we move things around to achieve a better separation of concepts across packages.
There was a problem hiding this comment.
@timholy did you have a chance to look into this? Can we migrate label_components to somewhere else? Maybe here?
There was a problem hiding this comment.
Anyone has an opinion on this? What should be the reorganization here? I vote to move label_components to some sub-package of Images.jl, not sure where it should go though, if here or some new package like ImageGraphs.jl
There was a problem hiding this comment.
Hi Julio,
I am working on bringing the entire Images ecosystem into a new unified API. For an example see: https://github.com/zygmuntszpak/ImageContrastAdjustment.jl.
I'm also currently working on an ImageComponentAnalysis package in collaboration with some great undergraduate students (https://github.com/zygmuntszpak/ImageComponentAnalysis.jl). The current label_components would be a natural fit for that package. However, its API would change slightly. It would be associated with the algorithm Generic(), that is, label_components(Generic(), ...). This is to differentiate it from other component labelling algorithms such as contour tracking, i.e. label_components(ContourTracking(),...) which are 2D only and assume 8-connectivity.
The ImageComponentAnalysis is under active development, so not yet ready to be tagged.
There was a problem hiding this comment.
Lovely @zygmuntszpak ! I am looking forward to see the results. ❤️ Do you have the unified API documented somewhere already? I may have missed the recent discussions on the issue tracker.
You mean that the entire Images.jl project is under a major reorganization of concepts?
There was a problem hiding this comment.
We started a discussion here: JuliaImages/Images.jl#772 but I haven't updated my latest thought there yet. Because some things in Images are so intertwined, and because the API means breaking changes etc. I decided that I would just quietly work on trying to get everything into one cohesive hole and once there is something tangible but unified, let people make recommendations for changes to details such as function names, argument order etc.
|
@juliohm Should we fix the tests by adding |
|
I think that is not a good solution given that Images.jl depends on
ImageMorphology.jl. Do you need it merged for some specific reason ? I
would just use the function locally and then add a note saying that it will
be available in Images.jl at some point. That way you can continue with
your work. Sorry for the delay in incorporating your changes. Let's wait a
bit more on the above mentioned work by other members of JuliaImages
…On Mon, Feb 11, 2019, 10:32 Rohit Kumar ***@***.*** wrote:
@juliohm <https://github.com/juliohm> Should we fix the tests by adding
Pkg.add("Images") to the CI files for now?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#9 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ADMLbZiadfz-Pu79Xooz9wdD3tduwGhpks5vMWLEgaJpZM4Zkqfv>
.
|
|
It would be great to have this feature as it one of the few in the comparison table that has no Julia equivalent: https://juliaimages.org/latest/api_comparison/ |
|
Since Future devs on |
|
@juliohm Is it good for merging now? |
|
Previous comments are already resolved; merge it since the test is passed without introducing P.S. I made some edits on indentation and typos such as |
implemented in JuliaImages/ImageMorphology.jl#9
|
Thank you Johnny for taking care of it.
…On Thu, May 30, 2019, 12:07 Johnny Chen ***@***.***> wrote:
Previous comments are already resolved; merge it since the test is passed
without introducing Images.jl dependency.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#9?email_source=notifications&email_token=AAZQW3LGGJQ5SGRVJYN4G2DPX7UTTA5CNFSM4GMSU7X2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODWSSV6Y#issuecomment-497363707>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAZQW3MMM7QVZB6CSTQTWIDPX7UTTANCNFSM4GMSU7XQ>
.
|
This function can be used to fill objects of a certain size with some specified value.
This function can be used to fill objects of a certain size with some specified value.
This function can be used to fill objects of a certain size with some specified value.
This function can be used to fill objects of a certain size with some specified value.
@juliohm Also I am thinking of adding a parameter
descriptors, which when true will give some stats of the connected components like: their sizes, their centeroid etc. What's your opinion on this?