Fix mapwindow for offset arrays #160

yha · 2020-03-23T01:41:03Z

The deleted assertions fail due to JuliaArrays/OffsetArrays.jl#104. As a workaround, I tried

@assert (imgr .+ kmin)[idx] ⊆ fullimgr
@assert (imgr .+ kmax)[idx] ⊆ fullimgr

instead, but that fails due to JuliaLang/julia#35225 when idx turns out empty.

codecov · 2020-03-23T02:10:15Z

Codecov Report

Merging #160 into master will increase coverage by 0.07%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #160      +/-   ##
==========================================
+ Coverage   90.99%   91.06%   +0.07%     
==========================================
  Files           9        9              
  Lines        1232     1231       -1     
==========================================
  Hits         1121     1121              
+ Misses        111      110       -1

Impacted Files	Coverage Δ
src/mapwindow.jl	`85.50% <100.00%> (+0.41%)`	⬆️

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 14f174d...5bab482. Read the comment docs.

johnnychen94 · 2020-03-23T03:53:37Z

I think the assertation makes sense and should not be removed. The fact that the tests pass is more concerning IMO...

With JuliaArrays/OffsetArrays.jl#105 and the following patch, the assertation passes, but there're some new errors in the added tests.

https://github.com/JuliaImages/ImageFiltering.jl/blob/master/src/mapwindow.jl#L200

function _indexof(r::AbstractRange, x)
    T = eltype(r)
    @assert x ∈ r
-	i = ont(T) + T((x - first(r)) / step(r))
+   i = first(axes(r)[1]) + T((x - first(r)) / step(r))
    @assert r[i] == x
    i
end

~~Update: Perhaps TiledIterations also needs some more update for OffsetArray 1.0~~

…ces in broadcasting)

yha · 2020-05-25T13:56:41Z

Updated version, which should work assuming JuliaArrays/OffsetArrays.jl#105 (and includes the assertions).
(I believe the previous version without the assertions was also technically correct, but in a weird way, with methods not doing exactly what their name suggests for offset arrays, and relying subtly on JuliaArrays/OffsetArrays.jl#104).

This version has axes(mapwindow(..., indices=2:7),1) == 1:6, which is what the docs says it should do, rather than 2:7 as before. This is also more consistent the behavior of non-unit-ranges, e.g., indices=2:2:8. I changed one test as a result, which previously tested for behavior which contradict the docs.

jw3126 · 2020-05-25T14:29:49Z

test/mapwindow.jl

+                                                            (dim,a) in enumerate(arrays)
+            offsets = ntuple(_->offset,dim)
+            windows = ntuple(_->window,dim)
+            @test OffsetArray(mapwindow(f,a,windows),offsets) == mapwindow(f,OffsetArray(a,offsets),windows)


I think this is a good test and we should have it. Just asking, is this test different from behavior on master? Could also add a case where there is no handcrafted version for f. E.g. f = x -> sum(x)

The behavior on master is an assertion error when the input array has non-zero offset.
The original version of this PR simply removed these assertions and loosened some type checks, which happens to work "accidentally", but would stop working once JuliaArrays/OffsetArrays.jl#104 is fixed (e.g. JuliaArrays/OffsetArrays.jl#105 is merged).
The current version is the opposite: it depends on JuliaArrays/OffsetArrays.jl#105 to work.

Not sure what you mean about handcrafted version for f. AFAICT there is no specialized implementation of mapwindow for minimum or maximum.

Thanks for clarifying. I do not remember for which functions there is a specialized implementation. Even if there is none for minimum/maximum right now, it could potentially be added in future, so I believe adding a lambda f won't hurt.

jw3126 · 2020-05-25T14:31:25Z

test/mapwindow.jl

@@ -115,7 +129,7 @@ using ImageFiltering: IdentityUnitRange
        @test expected == @inferred mapwindow!(f,out,img,window,indices=imginds)
    end
    for (inds, kw) ∈ [((Base.OneTo(3),), (border="replicate", indices=2:2:7)),
-                        ((2:7,), (indices=2:7,)),
+                        (axes(2:7), (indices=2:7,)),


I do not have an opinion on whether this is a bug fix or breaking change.

jw3126

Basically looks good to me, see comments above.

yha · 2020-05-25T15:08:45Z

Basically looks good to me, see comments above.

Note this should probably be merged only after JuliaArrays/OffsetArrays.jl#105, with a corresponding compat entry.

johnnychen94 · 2020-05-26T01:51:34Z

Sorry that I was too busy these days to take care of this PR. I'll find some time to work on JuliaArrays/OffsetArrays.jl#105 this later.

yha · 2020-05-26T13:45:29Z

Added tests for anonymous functions and various border styles. Also fixed the axes for Inner() which I apparently broke.
Caught some new failure cases which already fail in master. I'll open a new issue for those.

johnnychen94

The changes look reasonable and unharmful to me and the test passes locally

Reviewing ImageFiltering.jl is still quite challenging to me, it would be the best if we can get @timholy involved before merging this.

To adapt JuliaArrays/OffsetArrays.jl#114

johnnychen94 · 2020-06-18T00:08:24Z

reopen to retrigger the test

johnnychen94 · 2020-06-18T00:45:08Z

The failure in nightly is because I retriggered the CI too quickly that PkgServer (enabled by default since Julia 1.5) didn't get updated and thus didn't found OffsetArrays v1.1.0.

There seem to be other issues in the nightly test: https://github.com/JuliaCI/NanosoldierReports/blob/master/pkgeval/by_date/2020-06/13/logs/ImageFiltering/1.6.0-DEV-6b2ffd3913.log

yha · 2020-06-18T12:58:12Z

There seem to be other issues in the nightly test: https://github.com/JuliaCI/NanosoldierReports/blob/master/pkgeval/by_date/2020-06/13/logs/ImageFiltering/1.6.0-DEV-6b2ffd3913.log

This seems to be before the fix in #174.

johnnychen94 · 2020-06-18T13:40:13Z

It seems to be only an CI issue, the test passes locally for nightly build (mac and Linux).

I believe this PR is ready to merge and it's being unfairly delayed for quite a long time, so I plan to merge this next Monday if there are no further comments.

timholy · 2020-06-18T20:14:06Z

Let's try re-running CI

timholy

I haven't independently dug into the causes of the failures, so I am not fully up-to-speed on this, but by the weekend I'll try to take a more care. Meanwhile, the test alone here is fantastic!

timholy · 2020-06-18T20:21:53Z

src/mapwindow.jl

@@ -72,14 +73,18 @@ function mapwindow(f, img, window; border="replicate",
              resolve_imginds(indices))
 end

+# a unit range `r` having `r == axes(r)` which keeps its axes with
+# broadcasting (e.g., `axes(r.+1) == axes(r)`), unlike Base.IdentityUnitRange
+self_offset(r::AbstractUnitRange) = OffsetArrays.IdOffsetRange(1:length(r), first(r)-one(eltype(r)))


I'm not exactly sure I understand what you're trying to do. It doesn't quite do what it claims in the comment, for example

julia> r2 = OffsetArrays.IdOffsetRange(5:10, -3) 2:7 julia> r3 = self_offset(r2) 2:7 julia> first(r3) 2 julia> first(axes(r2, 1)) -2

What's the underlying goal? Would you like to create a range that preserves the values and indices of an original AbstractUnitRange but which behaves as described under broadcasting?

Or is this a situation where we can essentially ignore certain axes? For instance, we might imagine ignoring the axes of imginds, although alternatively they might specify the axes of the output.

I suppose the comment is confusing because it uses the name r for the output of the function when it's also as the name of the function parameter. It should really say that s = self_offset(r) has the same values as r but has itself as indices: collect(s) == collect(r) and axes(s) == (s,) (in your example, axes(r3) == (2:7,) == (r3,)).

Fixed the comment

src/mapwindow.jl

test/mapwindow.jl

yha · 2020-06-29T09:49:31Z

Bump

johnnychen94 · 2020-07-04T07:56:34Z

Hmmm... I'll merge this in days if there are no further comments

johnnychen94 · 2020-07-09T08:44:09Z

Given that @timholy is quite busy these days for a second-round comment, I'm merging this.

@yha This is really a nice and tough job! I trust you know how things going internally. Also, the test looks great to me, so let's test this patch in the wild.

Sorry again for the long delay here, once the test passes in master, I'll tag a release for this.

johnnychen94 mentioned this pull request Mar 23, 2020

[WIP] preserve offset when broadcasting IdOffsetRange JuliaArrays/OffsetArrays.jl#105

Closed

Fix mapwindow for offset arrays

77cb511

yha force-pushed the mapwindow-offset branch from 4493a06 to 77cb511 Compare May 25, 2020 12:57

Fix assuming merging of OffsetArrays#105 (IdOffsetRange preserve indi…

ef313e0

…ces in broadcasting)

jw3126 reviewed May 25, 2020

View reviewed changes

jw3126 approved these changes May 25, 2020

View reviewed changes

Fix output axes for border=Inner(), test with anonymous functions

92a4aa8

yha mentioned this pull request May 26, 2020

mapwindow failure cases #169

Open

johnnychen94 approved these changes Jun 12, 2020

View reviewed changes

johnnychen94 added 2 commits June 18, 2020 07:29

update OffsetArrays dependency

31c3312

To adapt JuliaArrays/OffsetArrays.jl#114

Merge branch 'master' into mapwindow-offset

7663f48

johnnychen94 closed this Jun 18, 2020

johnnychen94 reopened this Jun 18, 2020

johnnychen94 closed this Jun 18, 2020

johnnychen94 reopened this Jun 18, 2020

timholy closed this Jun 18, 2020

timholy reopened this Jun 18, 2020

timholy reviewed Jun 18, 2020

View reviewed changes

Clearer comment. Fix eltype in _indexof.

5bab482

johnnychen94 requested a review from timholy June 29, 2020 11:06

johnnychen94 merged commit b58b139 into JuliaImages:master Jul 9, 2020

juliohm mentioned this pull request Jul 9, 2020

mapwindow with border option not implemented? #175

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix mapwindow for offset arrays #160

Fix mapwindow for offset arrays #160

yha commented Mar 23, 2020

codecov bot commented Mar 23, 2020 •

edited

Loading

johnnychen94 commented Mar 23, 2020 •

edited

Loading

yha commented May 25, 2020

jw3126 May 25, 2020

yha May 25, 2020

jw3126 May 25, 2020

jw3126 May 25, 2020

jw3126 left a comment

yha commented May 25, 2020

johnnychen94 commented May 26, 2020

yha commented May 26, 2020

johnnychen94 left a comment •

edited

Loading

johnnychen94 commented Jun 18, 2020

johnnychen94 commented Jun 18, 2020 •

edited

Loading

yha commented Jun 18, 2020

johnnychen94 commented Jun 18, 2020 •

edited

Loading

timholy commented Jun 18, 2020

timholy left a comment

timholy Jun 18, 2020

yha Jun 18, 2020

yha Jun 24, 2020

yha commented Jun 29, 2020

johnnychen94 commented Jul 4, 2020

johnnychen94 commented Jul 9, 2020 •

edited

Loading

Fix mapwindow for offset arrays #160

Fix mapwindow for offset arrays #160

Conversation

yha commented Mar 23, 2020

codecov bot commented Mar 23, 2020 • edited Loading

Codecov Report

johnnychen94 commented Mar 23, 2020 • edited Loading

yha commented May 25, 2020

jw3126 May 25, 2020

Choose a reason for hiding this comment

yha May 25, 2020

Choose a reason for hiding this comment

jw3126 May 25, 2020

Choose a reason for hiding this comment

jw3126 May 25, 2020

Choose a reason for hiding this comment

jw3126 left a comment

Choose a reason for hiding this comment

yha commented May 25, 2020

johnnychen94 commented May 26, 2020

yha commented May 26, 2020

johnnychen94 left a comment • edited Loading

Choose a reason for hiding this comment

johnnychen94 commented Jun 18, 2020

johnnychen94 commented Jun 18, 2020 • edited Loading

yha commented Jun 18, 2020

johnnychen94 commented Jun 18, 2020 • edited Loading

timholy commented Jun 18, 2020

timholy left a comment

Choose a reason for hiding this comment

timholy Jun 18, 2020

Choose a reason for hiding this comment

yha Jun 18, 2020

Choose a reason for hiding this comment

yha Jun 24, 2020

Choose a reason for hiding this comment

yha commented Jun 29, 2020

johnnychen94 commented Jul 4, 2020

johnnychen94 commented Jul 9, 2020 • edited Loading

codecov bot commented Mar 23, 2020 •

edited

Loading

johnnychen94 commented Mar 23, 2020 •

edited

Loading

johnnychen94 left a comment •

edited

Loading

johnnychen94 commented Jun 18, 2020 •

edited

Loading

johnnychen94 commented Jun 18, 2020 •

edited

Loading

johnnychen94 commented Jul 9, 2020 •

edited

Loading