RFC: strided mapwindow #45

jw3126 · 2017-11-09T21:04:32Z

See #44. Here is a first implementation. The API is a bit cumbersome and needs to be improved. Currently you have to call

mapwindow(f,img,window,border,imginds)

Any suggestions for a better API? I would also like to add two convenience functions:

strided(inds, stride) converts a stride into something that can be passed to mapwindow:

mapwindow(f,...,strided(indices(img), (2,3))) = mapwindow(f,...)[1:2:end,1:3:end]

mappool(f,img,window) which does image pooling. This is the special case, where the stride is the size of the window.

mappool(f,img,window,...) = mapwindow(f,img,...,strided(indices(img), map(length,window))

timholy · 2017-11-14T12:32:50Z

Sorry for the delay here; I've been mulling over the API issues, and I agree it's not entirely straightforward. I keep returning to something like:

dest = PooledArray{T}(inds)
mapwindow!(f, dest, img, window, border)

and possibly a convenience method

dest = pooledarray(f, img, window, inds)

for the automatic computation of the eltype.

But to be honest I'm not convinced that's any better.

timholy

This is really elegant work. 👍

Do you think some of the in-code @asserts can be turned into tests? I'm not against leaving them in, though I'd want to know about their performance impact (I think they get eliminated with julia -O3).

timholy · 2017-11-14T12:43:52Z

test/runtests.jl

@@ -5,14 +5,14 @@ aif = detect_ambiguities(ImageFiltering, Kernel, KernelFactors, Base)
 asa = detect_ambiguities(StaticArrays, Base)
 @test isempty(setdiff(aif, asa))

+include("mapwindow.jl")


Is this deliberate or simply a temporary convenience?

Just a convenience, I can restore previous order if this is finished.

timholy · 2017-11-14T12:44:47Z

test/mapwindow.jl

+            (mean, randn(10), (-1:1,), (1:2:8,)),
+            (mean, randn(10,5), (-1:1,0:0), (1:2:8,1:3)),
+        ]
+        border = ImageFiltering.borderinstance("replicate")


Can we not just pass "replicate" directly?

Yes we can!

jw3126 · 2017-11-14T20:07:33Z

None of the @assert is inside a hot loop, so I don't think they will have a performance impact. I could turn them into tests, but these are implementations details I think assertions are much more convenient then tests here.

jw3126 · 2017-11-14T20:14:21Z

Introducing a new type of array seems a bit heavyweight for this. Is turning border into a keyword argument an option?

mapwindow(f,img,window,imginds; border=myborder)

At least border does not influence the return type AFAICT.

mapwindow!

seems to be useful also independently of this PR.

timholy · 2017-11-14T20:51:39Z

At least border does not influence the return type AFAICT.

It does for Fill, NoPad, and Inner. EDIT: sorry, probably doesn't affect the return type, just the dispatch. Probably not relevant, performance-wise.

Introducing a new type of array seems a bit heavyweight for this.

I agree. The only justification is whether it would be useful for "documenting" the results later. For example, if someone hands you an array that is a result of some pooling operation, does it help you to know where each "pixel" (result on a chunk) came from? Or do you not care?

And yes, mapwindow! is probably independently useful no matter what we decide here.

jw3126 · 2017-11-14T21:50:14Z

does it help you to know where each "pixel" (result on a chunk) came from? Or do you not care?

Depends on the application. Most of the time I just want to detect, whether some property is holds in an image. For this I don't care about the original pixels. But if you search something in an image, this sounds useful. In that case you might also want to chain some mapwindow! calls? Not sure. Also if you are interested in the original pixels, would you also like to remember windowsize?

Finally one might want to use a strided mapwindow! with another array type as dest. So we would probably need

struct PooledArray{T,N,A,I} <: AbstractArray{T,N}
    data::A
    indices::I
end

This is even more heavyweight and looks a lot like an AxisArray.

Here is a list of all options I can think of:

Introduce new array type PooledArray that wraps the output and use mapwindow!
Introduce new array type that wraps the input.
mapwindow(f, img, windowsize, border, indices)
Introduce a new function getindex_mapwindow(f,img,windowsize, indices, border)
Make indices a keyword argument. This would break type stability if we tried to support e.g. (1:3, 2) in future.

jw3126 · 2017-11-15T11:31:54Z

Another idea is making mapwindow lazy.

struct MapWindow{T,N,F,O,W,P} <: AbstractArray{T,N}
    f::F
    original_array::O
    windowsize::W
    pad::P
    # maybe also buffer?
end

MapWindow(f, img, windowsize)[1:4, 5:6]

mapwindow!(f,dest, img,windowsize, border) = copy!(dest, MapWindow(f,img,windowsize, border))

timholy · 2017-11-15T17:20:49Z

I generally love lazy, but here the problem is that it kind of violates one of the implicit "contracts" of AbstractArray: that elementwise access is cheap. I added something along these lines to ImageAxes in JuliaImages/ImageAxes.jl#19, and ended up not making it a subtype of AbstractArray. This of course makes some things more painful, but in general I think it's better to rely on "behavior" than it is on subtypes.

Of your list of 5 options above, I'd like to strike number 2. This is really a statement about what portion of the output you want to keep, so associating it with the input seems wrong. 4 seems somehow unnecessary. I find myself roughly equally attracted by 1, 3, and 5. I can go with the API you have now, if there isn't an obviously better alternative. Since I'm on the fence, I'd be happy to have you choose.

codecov · 2017-11-15T20:12:20Z

Codecov Report

Merging #45 into master will decrease coverage by 1.17%.
The diff coverage is 98.38%.

@@            Coverage Diff             @@
##           master      #45      +/-   ##
==========================================
- Coverage   90.12%   88.94%   -1.18%     
==========================================
  Files           8        8              
  Lines         992     1049      +57     
==========================================
+ Hits          894      933      +39     
- Misses         98      116      +18

Impacted Files	Coverage Δ
src/mapwindow.jl	`85.98% <98.38%> (+3.93%)`	⬆️
src/specialty.jl	`47.22% <0%> (-42.26%)`	⬇️

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 941acd0...1d7c4f1. Read the comment docs.

codecov · 2017-11-15T20:12:20Z

Codecov Report

Merging #45 into master will increase coverage by 0.35%.
The diff coverage is 94.8%.

@@            Coverage Diff             @@
##           master      #45      +/-   ##
==========================================
+ Coverage   90.12%   90.48%   +0.35%     
==========================================
  Files           8        8              
  Lines         992     1040      +48     
==========================================
+ Hits          894      941      +47     
- Misses         98       99       +1

Impacted Files	Coverage Δ
src/mapwindow.jl	`86.66% <94.8%> (+4.61%)`	⬆️

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 941acd0...4e1dd4e. Read the comment docs.

jw3126 · 2017-11-15T22:27:52Z

I will go with

mapwindow(f,img,window,border,imginds)

for now. Maybe over time something better will emerge.

timholy

Small tweaks, then I think this is ready for a squash-and-merge. Thanks for an outstanding contribution!

timholy · 2017-11-16T10:42:11Z

src/mapwindow.jl

+It works as follows:
+```julia
+mapwindow(f, img, window, border, (2:5, 1:2:7)) == mapwindow(f,img,window,border)[2:5, 1:2:7]
+```


Perhaps add "except more efficiently because it omits computation of the unused values."

timholy · 2017-11-16T10:44:22Z

src/mapwindow.jl

@@ -41,78 +48,176 @@ and then `mapwindow(f, img, (m,n))` should filter at the 75th quantile.

 See also: [`imfilter`](@ref).
 """
-function mapwindow(f, img::AbstractArray, window::Dims, args...; kwargs...)
+function mapwindow(f,img,window;kw...)
+    mapwindow(f, img, window, "replicate", kw...)


semicolon after "replicate". Usually stylistic, but here I think it's actually a bug (like it was in https://github.com/JuliaImages/ImageView.jl/pull/130/files).

Also perhaps add spaces between the arguments?

timholy · 2017-11-16T10:46:01Z

src/mapwindow.jl

-    mapwindow(f, img, (-h:h,), args...; kwargs...)
+    -h:h
+end
+resolve_window(window::AbstractArray) = resolve_window(img, (window...,))


where does the img come from?

timholy · 2017-11-16T10:52:15Z

src/mapwindow.jl

                    img::AbstractArray{T,N},
-                    window::Indices{N},
+                    window::NTuple{N,Range},


How about an AbstractUnitRange here, since I think we wouldn't properly handle a non-1 step if it were supplied.

jw3126 · 2017-11-16T18:18:47Z

Thanks for the feedback. There are still some corner cases, that I need to test/fix.

jw3126 · 2017-11-17T06:14:05Z

If there are no further objections, I will merge on Monday.

timholy · 2017-11-17T19:46:22Z

test/mapwindow.jl

+    end
+    @test mapwindow(mean, randn(10), (3,), "replicate", 2:2:7) isa Array
+    @test mapwindow(mean, randn(10), (3,), "replicate", 2:7) isa OffsetArray
+    @test mapwindow(mean, randn(10), (3,)) isa Array


Sorry I didn't notice this earlier, but over time I've gotten more skeptical about testing the output type directly (this hit me with a vengance in JuliaImages/ImageCore.jl#52). Can something equivalent be done by testing the indices?

Yeah I don't like these tests either. Testing the indices is more sane.

timholy · 2017-11-21T16:07:36Z

Thanks so much, this is fantastic!

timholy reviewed Nov 14, 2017

View reviewed changes

jw3126 mentioned this pull request Nov 15, 2017

add mapwindow benchmark #47

Merged

jw3126 force-pushed the mapwindow branch from 22e3cdd to 1d7c4f1 Compare November 15, 2017 20:12

timholy reviewed Nov 16, 2017

View reviewed changes

jw3126 added 2 commits November 16, 2017 22:26

bare bones strided mapwindow

52ceab2

cleanup mapwindow dispatch

a561418

jw3126 force-pushed the mapwindow branch from 8bbe6cf to a561418 Compare November 16, 2017 21:30

jw3126 mentioned this pull request Nov 16, 2017

mapwindow does not support Inner #48

Closed

timholy approved these changes Nov 17, 2017

View reviewed changes

refactor a mapwindow test

4e1dd4e

jw3126 merged commit d9a0a7a into JuliaImages:master Nov 20, 2017

jw3126 deleted the mapwindow branch November 20, 2017 19:28

jw3126 mentioned this pull request Nov 20, 2017

Add stride argument for mapwindow #44

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RFC: strided mapwindow #45

RFC: strided mapwindow #45

jw3126 commented Nov 9, 2017

timholy commented Nov 14, 2017

timholy left a comment

timholy Nov 14, 2017

jw3126 Nov 14, 2017

timholy Nov 14, 2017

jw3126 Nov 14, 2017

jw3126 commented Nov 14, 2017

jw3126 commented Nov 14, 2017

timholy commented Nov 14, 2017 •

edited

Loading

jw3126 commented Nov 14, 2017 •

edited

Loading

jw3126 commented Nov 15, 2017

timholy commented Nov 15, 2017

codecov bot commented Nov 15, 2017

codecov bot commented Nov 15, 2017 •

edited

Loading

jw3126 commented Nov 15, 2017

timholy left a comment

timholy Nov 16, 2017

timholy Nov 16, 2017

timholy Nov 16, 2017

timholy Nov 16, 2017

jw3126 commented Nov 16, 2017

jw3126 commented Nov 17, 2017

timholy Nov 17, 2017

jw3126 Nov 17, 2017

timholy commented Nov 21, 2017

RFC: strided mapwindow #45

RFC: strided mapwindow #45

Conversation

jw3126 commented Nov 9, 2017

timholy commented Nov 14, 2017

timholy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jw3126 commented Nov 14, 2017

jw3126 commented Nov 14, 2017

timholy commented Nov 14, 2017 • edited Loading

jw3126 commented Nov 14, 2017 • edited Loading

jw3126 commented Nov 15, 2017

timholy commented Nov 15, 2017

codecov bot commented Nov 15, 2017

Codecov Report

codecov bot commented Nov 15, 2017 • edited Loading

Codecov Report

jw3126 commented Nov 15, 2017

timholy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jw3126 commented Nov 16, 2017

jw3126 commented Nov 17, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timholy commented Nov 21, 2017

timholy commented Nov 14, 2017 •

edited

Loading

jw3126 commented Nov 14, 2017 •

edited

Loading

codecov bot commented Nov 15, 2017 •

edited

Loading