JuliaImages / ImageFiltering.jl

Julia implementations of multidimensional array convolution and nonlinear stencil operations
Other
99 stars 49 forks source link

add sliding_window #155

Closed jw3126 closed 3 years ago

jw3126 commented 4 years ago

My main motivation was to have multi argument mapwindow. Now you can do things like

map(f, sliding_window(arr1, window1), sliding_window(arr2, window2), arr3)

etc.

jw3126 commented 4 years ago

Any ideas what the CI fail is about? It only happens on 1.0, there I get unreachable reached.

timholy commented 4 years ago

One option would be to make this available only on 1.x, where x is the lowest minor release that does support it. If you decide to go that way, currently our travis testing only does 1.0, 1=latest stable release, and nightly, so you should add your 1.x to the travis script.

If you want to solve the problem that way let me know, then I will give this a review.

codecov[bot] commented 4 years ago

Codecov Report

Merging #155 into master will decrease coverage by 0.19%. The diff coverage is 88.31%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #155      +/-   ##
==========================================
- Coverage   91.59%   91.39%   -0.20%     
==========================================
  Files           9       10       +1     
  Lines        1213     1290      +77     
==========================================
+ Hits         1111     1179      +68     
- Misses        102      111       +9     
Impacted Files Coverage Δ
src/mapwindow.jl 86.59% <ø> (-0.60%) :arrow_down:
src/sliding_window.jl 88.05% <88.05%> (ø)
src/ImageFiltering.jl 83.33% <90.00%> (+3.33%) :arrow_up:
src/kernel.jl 100.00% <0.00%> (ø)

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 46c0445...67f373c. Read the comment docs.

jw3126 commented 4 years ago

One option would be to make this available only on 1.x, where x is the lowest minor release that does support it. If you decide to go that way, currently our travis testing only does 1.0, 1=latest stable release, and nightly, so you should add your 1.x to the travis script.

If you want to solve the problem that way let me know, then I will give this a review.

Yeah that sounds like a pragmatic solution.

timholy commented 4 years ago

Another approach would be to make this particular function available only on Julia > 1 with a suitable version check, and the docstring marked with

!!! compat "Julia 1.1"
    sliding_window is only available on Julia 1.1 and higher.

I can go either way, but I slightly prefer being able to continue to support Julia 1.0 for as long as that is feasible.

jw3126 commented 4 years ago

Another approach would be to make this particular function available only on Julia > 1 with a suitable version check, and the docstring marked with

Done

timholy commented 4 years ago

I'll get to this as soon as a I can. If you haven't heard from me in a week, please ping again.

jw3126 commented 4 years ago

@timholy just a reminder to review this.

johnnychen94 commented 3 years ago

It occurs to me that some of the functionality here is overlapped with TiledIteration. Currently, that package only supports disjoin tiles, perhaps a better strategy is to enhance that package with sliding semantics?

julia> img1 = testimage("camera");

julia> img2 = testimage("mandril_gray");

julia> iter1 = TileIterator(axes(img1), (9, 9))
TileIterator((Base.OneTo(512), Base.OneTo(512)), (9, 9))

julia> iter2 = TileIterator(axes(img2), (9, 9))
TileIterator((Base.OneTo(512), Base.OneTo(512)), (9, 9))

julia> map(iter1, iter2) do R1, R2
           mse(view(img1, R1...), view(img2, R2...))
       end;
3249-element Array{Float32,1}:
...

We could support TileIterator on CartesianIndices and returns a CartesianIndices, in which case the output is still an "image"

We could support TileIterator on AbstractArray as img, so that it can be simplified to map(mse, TileIterator(img1, (9, 9)), TileIterator(img2, (9, 9)))

ImageFiltering is becoming unnecessarily complex and hard to maintain or enhance. My vision on ImageFiltering is to not adding internal array types into this package, and instead, maintaining a more generic version as an independent package.

johnnychen94 commented 3 years ago

With https://github.com/JuliaArrays/TiledIteration.jl/pull/23 merged, I believe this is no longer wanted?

jw3126 commented 3 years ago

Note that JuliaArrays/TiledIteration.jl#23 is not quite enough to cover this PR. For one a unit stride strategy is missing. And then this one does border handling + tiling, while TiledIteratation is only about the tiling. Nevertheless we can close this PR. While much of the functionality of this PR is still desirable, I would choose to implement it more on top of TiledIteratation now.