graph-based segmentation merge

[jsundram]

see conversation here: https://discourse.julialang.org/t/hierarchical-merge-on-a-region-adjacency-graph/51633/4

[timholy]

Sorry for the delay. This is great! Just minor issues before merging.

[timholy]

This is looking good. Two small issues left to discuss:

• it's conventional, but not required, that in mutating functions (ones ending with !), the first argument it typically the mutated entity. There are many exceptions though (e.g., map!). Style recommendations are documented here: https://docs.julialang.org/en/v1/manual/style-guide/#Write-functions-with-argument-ordering-similar-to-Julia-Base. Alternatively, you can make a case for the current ordering of invalidate_neighbors! and just add an extra note to the docstring about which argument(s) are mutated.
• this exports merge which will conflict with Base.merge. Since you have unambiguous argument types, should we just extend Base.merge rather than creating our own independent merge function?

[jsundram]

Thanks! re: outstanding points:

1. Sorry, I didn't notice your original suggestion to change the parameter order for invalidate_neighbors in addition to adding a ! to indicate mutation. Will fix.

2. It's a good point re: Base.merge that I didn't consider when naming the function. The semantics of Base.merge function seem to be taking 2 or more instances of a type and returning 1 instance of that type. Which is different than the semantics of merge here. The same function is called merge_hierarchical in scikit-image. I'm not sure if that's better? The intent of this merge function is similar to that of prune_segments in core.jl -- perhaps merge_segments is a reasonable name? I'm happy to follow your preference here, and am totally open to other suggestions. Would it make sense to rename the file to match whatever function name we settle on?

[timholy]

I appreciate your awareness of the issue "are these two functions really doing the same thing?" That's really important and kudos to you for noticing & caring.

There's one easy solution: don't export merge. Then it would be called ImageSegmentation.merge(args...). But you also make a good case for merge_segments, I like that quite a lot. I'm fine with whichever you prefer.

The filename is also optional. I'd be happy to see it renamed (that might be clearest), but I'll have no reservations whatsoever if you decide to keep it the same.

[timholy]

Sorry, @jsundram, I was about to hit merge and then noticed one more thing. If you're good with this you can just accept the suggestion. Once the tests run, are you OK with merging or is there anything else you want to change?

[jsundram]

Sorry, @jsundram, I was about to hit merge and then noticed one more thing. If you're good with this you can just accept the suggestion.

Accepted, thanks for that improvement!

Once the tests run, are you OK with merging or is there anything else you want to change?

I think this looks good. I have a couple of questions that might be better resolved in different PRs:

1. Does it make sense to replace SimpleWeightedGraph (in core.jl) with a MetaGraph with a property :weight on its edges? SimpleWeightedGraph has two issues that I noticed in the readme: a) zero-weight edges are discarded by add_edge! and b) adding or removing vertices or edges is not particularly performant. And also this issue https://github.com/JuliaGraphs/SimpleWeightedGraphs.jl/issues/66.
2. Should the list of functions, imports and includes be alphabetized in ImageSegmentation.jl and runtests.jl?

[timholy]

Great questions. I'm not very fresh on these things myself, but if memory serves SimpleWeightedGraph is based on sparse matrices and is indeed incredibly slow for additions/removals of edges. If the performance is limiting, I'd be very much in favor of a more performant alternative. But I confess I'm not very fresh on these things so you should take your own experience as a more reliable guide.

Thanks again for a terrific PR, @jsundram!

[jsundram]

@timholy -- I was just wondering; do I also need to bump the version number and make a new release, so that people can use the new code easily? I'm not sure how releases get made...

[timholy]

My fault, I should have made a release. I guess I might have been holding back wondering if you were wanting to tackle the first item in #64 (comment). It's not required, though; would you like me to make a release?

I'm not sure how releases get made...

Not much to it:

1. Someone needs to bump the version number. Since this is a new feature it should be 1.5.0 (1.4.9 would be for bugfixes only).
2. Someone with commit privileges needs to notify the bot, see for example 556eabb

How about this: if you're ready for a release now, submit a PR bumping the version; I'll merge it and do the rest. Or tell me to make a new release and I'll do it. (It's no trouble, it's <1min of work total.)

[jsundram]

Thanks!

I'll hold off on SimpleWeightedGraph -> MetaGraph change, since I don't (yet?) have any evidence that there are performance wins to be had as a result of that change.

I just submitted #65 to get the release train started.