Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Provide better support for manually-specified borders (fixes #85) #86

Merged
merged 4 commits into from
Nov 30, 2018

Conversation

timholy
Copy link
Member

@timholy timholy commented Nov 27, 2018

@zygmuntszpak, does this seem OK?

@zygmuntszpak
Copy link
Member

Thank you for fixing this. I will try to write some further border tests for you tomorrow so that we have a record that the other padding styles also work as expected.

@timholy
Copy link
Member Author

timholy commented Nov 27, 2018

Test failure on master is addressed by JuliaLang/julia#30164

@zygmuntszpak
Copy link
Member

I've written the following tests which you could add to your current set:

B = zeros(similar(A))
C = zeros(similar(A))
r1 = imfilter!(B, A, Kernel.gaussian((1,1),(3,3)), Fill(0))
r2 = imfilter!(C, A,  Kernel.gaussian((1,1),(3,3)), Fill(10))
@test r1[4,4] == r2[4,4]
@test r1[1,1] != r2[1,1]

B = zeros(similar(A))
C = zeros(similar(A))
r1 = imfilter!(B, A, Kernel.gaussian((1,1),(3,3)), Fill(0, (3,3)))
r2 = imfilter!(C, A,  Kernel.gaussian((1,1),(3,3)), Fill(10, (3,3)))
@test r1[4,4] == r2[4,4]
@test r1[1,1] != r2[1,1]

B = zeros(similar(A))
C = zeros(similar(A))
r1 = imfilter!(B, A, Kernel.gaussian((1,1),(3,3)), Fill(0, (3,3),(3,3)))
r2 = imfilter!(C, A,  Kernel.gaussian((1,1),(3,3)), Fill(10, (3,3),(3,3)))
@test r1[4,4] == r2[4,4]
@test r1[1,1] != r2[1,1]

B = zeros(similar(A))
C = zeros(similar(A))
r1 = imfilter!(B, A, Kernel.gaussian((1,1),(3,3)), Fill(0, [3,3],[3,3]))
r2 = imfilter!(C, A,  Kernel.gaussian((1,1),(3,3)), Fill(10, [3,3],[3,3]))
@test r1[4,4] == r2[4,4]
@test r1[1,1] != r2[1,1]


 g = collect(KernelFactors.gaussian(1,3))
r1 = imfilter(A, ( ImageFiltering.ReshapedOneD{2,0}(centered(g)),), Fill(0))
r2 = imfilter(A, ( ImageFiltering.ReshapedOneD{2,0}(centered(g)),), Fill(10))
@test r1[4,4] == r2[4,4]
@test r1[1,1] != r2[1,1]

r1 = imfilter(A, ( ImageFiltering.ReshapedOneD{2,0}(centered(g)),), Fill(0, (3,3)))
r2 = imfilter(A, ( ImageFiltering.ReshapedOneD{2,0}(centered(g)),), Fill(10, (3,3)))
@test r1[4,4] == r2[4,4]
@test r1[1,1] != r2[1,1]

r1 = imfilter(A, ( ImageFiltering.ReshapedOneD{2,0}(centered(g)),), Fill(0, (3,3),(3,3)))
r2 = imfilter(A, ( ImageFiltering.ReshapedOneD{2,0}(centered(g)),), Fill(10, (3,3),(3,3)))
@test r1[4,4] == r2[4,4]
@test r1[1,1] != r2[1,1]

r1 = imfilter(A, ( ImageFiltering.ReshapedOneD{2,0}(centered(g)),), Fill(0, [3,3],[3,3]))
r2 = imfilter(A, ( ImageFiltering.ReshapedOneD{2,0}(centered(g)),), Fill(10, [3,3],[3,3]))
@test r1[4,4] == r2[4,4]
@test r1[1,1] != r2[1,1]

r1 = imfilter(A, ( ImageFiltering.ReshapedOneD{2,0}(centered(g)),), Fill(0))
r2 = imfilter(A, ( ImageFiltering.ReshapedOneD{2,0}(centered(g)),), Fill(10))
@test r1[4,4] == r2[4,4]
@test r1[1,1] != r2[1,1]

r1 = imfilter(A, ( ImageFiltering.ReshapedOneD{2,1}(centered(g)),), Fill(0, (3,3)))
r2 = imfilter(A, ( ImageFiltering.ReshapedOneD{2,1}(centered(g)),), Fill(10, (3,3)))
@test r1[4,4] == r2[4,4]
@test r1[1,1] != r2[1,1]

r1 = imfilter(A, ( ImageFiltering.ReshapedOneD{2,1}(centered(g)),), Fill(0, (3,3),(3,3)))
r2 = imfilter(A, ( ImageFiltering.ReshapedOneD{2,1}(centered(g)),), Fill(10, (3,3),(3,3)))
@test r1[4,4] == r2[4,4]
@test r1[1,1] != r2[1,1]

r1 = imfilter(A, ( ImageFiltering.ReshapedOneD{2,1}(centered(g)),), Fill(0, [3,3],[3,3]))
r2 = imfilter(A, ( ImageFiltering.ReshapedOneD{2,1}(centered(g)),), Fill(10, [3,3],[3,3]))
@test r1[4,4] == r2[4,4]
@test r1[1,1] != r2[1,1]

I'd like to draw your attention to the following particular tests. They are unusual in that a plain-text
error message is displayed in the user, but a genuine error is not thrown. Instead the caller
obtains a matrix filled with zeros. I'm not sure if this is intended or not.

# These return a message that an error was thrown, but don't actually throw an
# error. Instead, they return a matrix filled with zeros. 
r1 = imfilter(A, Kernel.gaussian((1,1),(3,3)), Fill(0, (1,0)))
r1 = imfilter(A, Kernel.gaussian((1,1),(3,3)), Fill(0, (0,1)))
r1 = imfilter(A, Kernel.gaussian((1,1),(3,3)), Fill(0, (0,0)))

@codecov
Copy link

codecov bot commented Nov 30, 2018

Codecov Report

Merging #86 into master will increase coverage by 0.7%.
The diff coverage is 57.14%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master      #86     +/-   ##
=========================================
+ Coverage   61.86%   62.56%   +0.7%     
=========================================
  Files           8        8             
  Lines        1070     1074      +4     
=========================================
+ Hits          662      672     +10     
+ Misses        408      402      -6
Impacted Files Coverage Δ
src/kernelfactors.jl 59.66% <ø> (+2.52%) ⬆️
src/imfilter.jl 63.83% <100%> (-1.09%) ⬇️
src/border.jl 63.75% <33.33%> (-1.22%) ⬇️
src/utils.jl 53.57% <50%> (-1.99%) ⬇️
src/kernel.jl 82.47% <0%> (+12.37%) ⬆️

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 73e73bc...7afa780. Read the comment docs.

@timholy timholy merged commit 3b66231 into master Nov 30, 2018
@timholy timholy deleted the teh/borders branch November 30, 2018 15:46
@timholy
Copy link
Member Author

timholy commented Nov 30, 2018

I'd like to draw your attention to the following particular tests.

You must be using something other than Julia 1.x? zeros(::Array) isn't supported anymore, and those tests work as expected for me (and on CI).

@zygmuntszpak
Copy link
Member

I was using 0.7 but I was also using Juno which automatically launches Julia with multiple threads. When I launch Julia from the terminal and evaluate

 @test_throws DimensionMismatch imfilter(A, Kernel.gaussian((1,1),(3,3)), Fill(0, (0,0)))

the test passes.

On the other hand, with multiple threads one gets a message:

@test_throws DimensionMismatch imfilter(A, Kernel.gaussian((1,1),(3,3)), Fill(0, (0,0)))

Error thrown in threaded loop on thread 0: Base.DimensionMismatch(msg="requested indices (1:8, 0:9) and kernel indices (Base.Slice(-1:1), Base.Slice(0:0)) do not agree with indices of padded input, (Base.Slice(1:8), Base.Slice(1:8))")Test Failed
at none:1
  Expression: imfilter(A, Kernel.gaussian((1, 1), (3, 3)), Fill(0, (0, 0)))
    Expected: DimensionMismatch
  No exception thrown
ERROR: There was an error during testing

So one receives a message that an error was thrown in a loop in a thread, but a genuine exception is not thrown. Is this perhaps currently just a limitation of Julia?

@timholy
Copy link
Member Author

timholy commented Dec 4, 2018

Oh interesting. I get the same thing if I use

$  JULIA_NUM_THREADS=2 julia runtests.jl

from the Linux prompt. Issue: #87. No time to look at it now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants