Closed timholy closed 3 years ago
My first impression of findlocalextrema
is that it can be implemented quite easily with mapwindow
so living in ImageFiltering.jl sounds a natural choice to me.
# a slightly different implementation
function my_findlocalmaxima(X)
mask = mapwindow(CartesianIndices(X), (3, 3)) do Rp
p = X[Rp[OffsetArrays.center(Rp)...]]
all(q->p >= q, view(X, Rp))
end
out = CartesianIndices(X)[mask]
end
Move in progress. I do think we want to keep these since they have some advantages:
julia> img = rand(100,100);
julia> @btime findlocalmaxima($img);
378.800 μs (10013 allocations: 377.05 KiB)
julia> @btime my_findlocalmaxima($img);
3.532 ms (76365 allocations: 2.39 MiB)
(EDIT: and fixing an inferrability problem revealed by this demo reduces the time by more than another factor of 2.)
Aside from speed, findlocalmaxima
works on images that are too big to store in memory all at once, but that would be a lot harder to achieve with the mapwindow
solution.
What to do about imROF
? Is that an ImageFiltering.jl operation or an ImageNoise.jl operation?
What to do about imROF? Is that an ImageFiltering.jl operation or an ImageNoise.jl operation?
I'll make a new implementation in ImageNoise. https://github.com/JuliaImages/Images.jl/issues/755 years later I think I'm ready to make a fastest implementation :)
Here's a question: the package only requires ImageFiltering 0.6 or higher, but the tests require ImageFiltering 0.7. How should we handle the [compat]
?
Pkg resolver will automatically use the latest version, right? Or are you worried about the tests on ImageFiltering v0.6? I think this is an issue of all packages; IIUC Julia doesn't have a test tool to run thorough compatibility tests.
Right, as long as there's no blocker then it will use the latest and pass the tests, which is why I left the [compat]
at the package requirement rather than the test requirement.
Should I add a comment at the top of the watershed test file? Or are we good with interpreting the problem and should just keep as-is?
Should I add a comment at the top of the watershed test file? Or are we good with interpreting the problem and should just keep as-is?
Instead of leaving a comment, we should mark them as conditional test groups using if branch. That if we ever have the chance to bring a comprehensive compatibility test, we don't see a failure there.
But this isn't something important right now IMHO
Sounds good. Ready to merge and tag a new release?
It's your call 😄
:laughing: I will go through the examples in the docs and make sure they all still work...
Yep! Small differences for felzenswalb and perhaps others (mostly likely because of the division by 3 in _abs2
), so we may have to adjust a few constants, but other than that this looks good. I may wait to update the docs until we have a compatible version of Images, since the examples still load Images.
Hmm, it occurs to me that the division by 3 is effectively breaking for older scripts. The transition to ImageCore 0.9 (and specifically, the new consistency in abs2
) has more implications than I expected! Should the next release be a 2.0 release? In which case we want to wait until we make more breaking API changes.
This drops Images entirely, but doesn't pass tests. There are three issues, all in the watershed test:
magnitude_phase
, which AFAICT is mostly to generate a box. I've prepared a substitute.findlocalminima
is still needed from Images. I'm tempted to move nearly everything out of Images and make it a pure meta-package. If people like that, I can think of two places, ImageBase and ImageFiltering. Thoughts?~closes #71