Closed yha closed 4 years ago
Merging #160 into master will increase coverage by
0.07%
. The diff coverage is100.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%) |
:arrow_up: |
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.
I think the assertation makes sense and should not be removed. The fact that the tests pass is more concerning IMO...
With https://github.com/JuliaArrays/OffsetArrays.jl/pull/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~
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.
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.
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.
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.
reopen to retrigger the test
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
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.
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.
Let's try re-running CI
Bump
Hmmm... I'll merge this in days if there are no further comments
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.
The deleted assertions fail due to JuliaArrays/OffsetArrays.jl#104. As a workaround, I tried
instead, but that fails due to JuliaLang/julia#35225 when
idx
turns out empty.