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

Qualify use of centered #36

Merged
merged 1 commit into from
Aug 8, 2021

Conversation

angusmoore
Copy link
Contributor

centered is exported by both ImageFiltering and OffsetArrays. This causes issues, as both packages are using within, so the name is ambiguous. Fix by (1) explicitly calling OffsetArrays.centered (2) import rather than using ImageFiltering.

Assume this bug was introduced by OffsetArrays recently newly exporting centered (introduced here: JuliaArrays/OffsetArrays.jl#242)

For clarity, the problem this PR fixes is this (in a fresh environment, install Images and TestImages and run the example in the ImageQualityIndexes readme):

]st
[916415d5] Images v0.24.1
[5e47fb64] TestImages v1.6.0

julia> img = testimage("cameraman") .|> float64
noisy_img = img .+ 0.1 .* randn(size(img))
assess_ssim(noisy_img, img) 

WARNING: both ImageFiltering and OffsetArrays export "centered"; uses of it in module ImageQualityIndexes must be qualified
ERROR: UndefVarError: centered not defined
Stacktrace:
 [1] SSIM(kernel::OffsetArrays.OffsetVector{Float64, Vector{Float64}}, W::Tuple{Float64, Float64, Float64}; crop::Bool)
   @ ImageQualityIndexes ~/.julia/packages/ImageQualityIndexes/XJPki/src/ssim.jl:57
 [2] assess_ssim(x::Matrix{Gray{Float64}}, ref::Matrix{Gray{Float64}}; crop::Bool)
   @ ImageQualityIndexes ~/.julia/packages/ImageQualityIndexes/XJPki/src/ssim.jl:76
 [3] assess_ssim(x::Matrix{Gray{Float64}}, ref::Matrix{Gray{Float64}})
   @ ImageQualityIndexes ~/.julia/packages/ImageQualityIndexes/XJPki/src/ssim.jl:76
 [4] top-level scope
   @ REPL[11]:1

 is exported by both ImageFiltering and OffsetArrays. This causes issues, as both packages are using within, so the name is ambiguous. Fix by (1) explicitly calling OffsetArrays.centered (2) import rather than using ImageFiltering.
@johnnychen94
Copy link
Member

Thank you so much for reporting and providing a fix for it.

OffsetArrays.centered is intentionally not exported to avoid this case but a recent deprecation has accidentally made it exported. JuliaArrays/OffsetArrays.jl#252 reverts this export.

But generally, we're planning to replace ImageFiltering.centered with OffsetArrays.centered so yeah we still want it.

The error seems unrelated, I'll fix it in a separate PR.

@codecov
Copy link

codecov bot commented Aug 8, 2021

Codecov Report

Merging #36 (f2c2998) into master (bb4e7d1) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master      #36   +/-   ##
=======================================
  Coverage   98.72%   98.72%           
=======================================
  Files           5        5           
  Lines         157      157           
=======================================
  Hits          155      155           
  Misses          2        2           
Impacted Files Coverage Δ
src/msssim.jl 100.00% <100.00%> (ø)
src/ssim.jl 100.00% <100.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 bb4e7d1...f2c2998. Read the comment docs.

@johnnychen94
Copy link
Member

The rest of the test failure in the nightly builds is fixed by #37

Thank you again for opening this PR.

@johnnychen94 johnnychen94 merged commit 84fd4b4 into JuliaImages:master Aug 8, 2021
@angusmoore angusmoore deleted the offsetarrays-centered branch August 8, 2021 06:12
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