JuliaImages / ImageSegmentation.jl

Partitioning images into meaningful regions
Other
47 stars 23 forks source link

Added accessor functions #13

Closed annimesh2809 closed 7 years ago

annimesh2809 commented 7 years ago

Functions for accessing members of SegmentedImage. These should be useful considering the results of fuzzy c-means are not of type SegmentedImage but FuzzyCMeansResult. Dispatch would help in providing a clean API for accessing information regarding the segments for both the types.

codecov[bot] commented 7 years ago

Codecov Report

Merging #13 into master will decrease coverage by 0.15%. The diff coverage is 91.66%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #13      +/-   ##
==========================================
- Coverage   98.39%   98.23%   -0.16%     
==========================================
  Files           9        9              
  Lines         499      511      +12     
==========================================
+ Hits          491      502      +11     
- Misses          8        9       +1
Impacted Files Coverage Δ
src/region_growing.jl 100% <ø> (ø) :arrow_up:
src/fast_scanning.jl 100% <ø> (ø) :arrow_up:
src/ImageSegmentation.jl 100% <ø> (ø) :arrow_up:
src/core.jl 98.93% <91.66%> (-1.07%) :arrow_down:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 704170b...54212b2. Read the comment docs.

annimesh2809 commented 7 years ago

@timholy @tejus-gupta Are the function names descriptive or should we change them? Once the function names are decided, then we can update the tests to use these accessor functions.

tejus-gupta commented 7 years ago

:+1: for function names. I don't think we need to update test cases for the algorithms. It would be more useful to have separate tests for the accessors and algorithms.

annimesh2809 commented 7 years ago

Should we merge this?

timholy commented 7 years ago

Whenever you're ready. I wasn't sure if you were still intending to add them for FuzzyCMeansResult, so I didn't want to hit merge myself.

annimesh2809 commented 7 years ago

Do the dispatch methods look fine? I couldn't find something analogous to labels_map for FuzzyCMeansResult so I did not dispatch it.

timholy commented 7 years ago

:+1: Maybe some tests for the FuzzyCMeans? Or is that waiting on a tag on Clustering?

annimesh2809 commented 7 years ago

Yes, I have added the tests for FuzzyCMeans in an already merged PR.

Docs need to be added for Kmeans and FuzzyCMeans. I will add them in a separate PR.

Should we register this package after we merge this PR?

timholy commented 7 years ago

Sorry, I just meant that it would be good to test the accessor functions for FuzzyCMeanResult. But if you can't really do that now because Clustering needs a tag, we can wait to add those tests.

timholy commented 7 years ago

And I like the idea of registering it after this merges!

annimesh2809 commented 7 years ago

Hmm... It seems like the tests are failing as I have used FuzzyCMeansResult and Clustering is not tagged yet. Will a failing build cause problems while registering? If yes, then I can remove the dispatched accessor methods for FuzzyCMeansResult now and add them (along with tests) once Clustering is tagged. Or we can also use the !isdefined(Clustering, :FuzzyCMeansResult) trick similar to hash?

timholy commented 7 years ago

Hopeful https://github.com/JuliaLang/METADATA.jl/pull/10940 will be merged soon, basically as soon as it passes CI. I can do the merge if I'm the first to notice.

timholy commented 7 years ago

OK, it's merged.