Closed jsundram closed 3 years ago
This is looking good. Two small issues left to discuss:
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.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?Thanks! re: outstanding points:
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.
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?
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.
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:
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.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!
@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...
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 https://github.com/JuliaImages/ImageSegmentation.jl/pull/64#issuecomment-783616167. 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:
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.)
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.
see conversation here: https://discourse.julialang.org/t/hierarchical-merge-on-a-region-adjacency-graph/51633/4