JuliaTesting / ReferenceTests.jl

Utility package for comparing data against reference files
https://juliatesting.github.io/ReferenceTests.jl/latest/
Other
82 stars 14 forks source link

enable image display using ImageShow #41

Open johnnychen94 opened 4 years ago

johnnychen94 commented 4 years ago

This patch only changes the render behavior for images when ImageShow is loaded in a MIME envionment(e.g, jupyter, juno, vscode):

julia> using ReferenceTests, TestImages, ImageShow

julia> img = testimage("mandrill")

julia> @test_reference "lena.png" img
┌ Warning: test fails because size(ref) (256, 256) != size(x) (512, 512)
└ @ ReferenceTests d:\Julia\ReferenceTests.jl\src\equality_metrics.jl:38
Test for "lena.png" failed.
- REFERENCE --------|--------- ACTUAL -
Replace reference with actual result (path: D:\Julia\ReferenceTests.jl\lena.png)? [y/n]

image

@juliohm I believe this is what you want in #26 ?

There're some binary dependencies that we don't like to have:

johnnychen94 commented 4 years ago

I have no idea how this function could be coded as a test case... Perhaps just make sure this MIME"image/png" method got called?

coveralls commented 4 years ago

Coverage Status

Coverage decreased (-2.4%) to 87.121% when pulling 0f2af14b72a7f30e2569537e6520b82ef9f3199b on jc/display into 712f30fb7f38395b4cd96045ac15e5b5a3c21148 on master.

coveralls commented 4 years ago

Coverage Status

Coverage decreased (-1.1%) to 81.183% when pulling 55407417ee2233182bc13907bfd4f08806db7525 on jc/display into 46d650a2efc73177bc9df7408a579c78488c6849 on master.

johnnychen94 commented 4 years ago

We could also add using ImageShow in ReferenceTests to enable this feature by default.

P.S. ImageInTerminal also changes how images show in REPL in the same manner:

julia> using ImageCore

julia> img = rand(Gray{N0f8}, 4, 4)
4×4 Array{Gray{N0f8},2} with eltype Gray{Normed{UInt8,8}}:
 Gray{N0f8}(0.133)  Gray{N0f8}(0.949)  Gray{N0f8}(0.992)  Gray{N0f8}(0.286)
 Gray{N0f8}(0.804)  Gray{N0f8}(0.624)  Gray{N0f8}(0.184)  Gray{N0f8}(0.514)
 Gray{N0f8}(0.063)  Gray{N0f8}(0.541)  Gray{N0f8}(0.337)  Gray{N0f8}(0.02)
 Gray{N0f8}(0.506)  Gray{N0f8}(0.78)   Gray{N0f8}(0.392)  Gray{N0f8}(0.89)

julia> using ReferenceTests

julia> img
4×4 Array{Gray{N0f8},2} with eltype Gray{Normed{UInt8,8}}:
████████
████████
████████
████████
johnnychen94 commented 4 years ago

Here comes to a decision I'd like to hear your voice @oxinabox @Evizero

There're two paths in front:

I prefer path two here since this package is designed for test stages, convenience could mean a lot; we won't like the way to manually add ImageShow into test stage's dependency.

oxinabox commented 4 years ago

I would like to avoid (any more) adding dependencies with binary dependencies. They just have too many modes of failure still. Do either of ImageInTerminal or ImageShow have binary dependencies (that we don't already depend on) in their chain?

johnnychen94 commented 4 years ago

Do either of ImageInTerminal or ImageShow have binary dependencies (that we don't already depend on) in their chain?

ImageShow requires FFTW and ImageInTerminal requires FFTW and SpecialFunctions. I believe this is acceptable?

With regard to the import time: @time using ReferenceTests

Just proposed to make FFTW a soft dependency of ImageCore https://github.com/JuliaImages/ImageCore.jl/pull/106, which could make this package more lightweight


I feel like there's a need to sort out the dispatching rules for render (e.g., add dispatch on MIME and remove BeforeAfterImage)

oxinabox commented 4 years ago

ImageShow requires FFTW and ImageInTerminal requires FFTW and SpecialFunctions. I believe this is acceptable?

Both of these have been annoying to me lately. Because of binary issues. So I'ld rather not. Those seem to be fixed but ...

johnnychen94 commented 4 years ago

If https://github.com/JuliaImages/ImageCore.jl/pull/106 got merged, then we don't require FFTW here, but SpecialFunctions is still required by ImageInTerminal via ImageTransformations.

oxinabox commented 4 years ago

I'ld be down with it, if that was done

johnnychen94 commented 4 years ago

With ImageCore v0.8.8 released, FFTW isn't required anymore, so I plan to just add ImageShow as a dependency to this package without Requires.jl

Here's the speed up in ReferenceTests side (master branch):

Updating `~/Desktop/Project.toml`
  [a09fc81d] ↓ ImageCore v0.8.10 ⇒ v0.8.7
  Updating `~/Desktop/Manifest.toml`
  [621f4979] + AbstractFFTs v0.5.0
  [3da002f7] ↓ ColorTypes v0.9.0 ⇒ v0.8.1
  [5ae59095] ↓ Colors v0.11.1 ⇒ v0.10.1
  [7a1cc6ca] + FFTW v1.2.0
  [f5851436] + FFTW_jll v3.3.9+3
  [a09fc81d] ↓ ImageCore v0.8.10 ⇒ v0.8.7
  [1d5cc7b8] + IntelOpenMP_jll v2018.0.3+0
  [856f044c] + MKL_jll v2019.0.117+0
johnnychen94 commented 4 years ago

It's not a trivial task to remove ImageTransformation from ImageInTerminal; for 24bit colors anti-alias restrict does improve the visual quality. So this is a future work -- after all, it's the status quo.

I don't have any ideas to test this PR, any suggestions?

oxinabox commented 4 years ago

I don't have any ideas to test this PR, any suggestions?

Mocking.jl Mock out the call that would display the image, and make sure it is hit at appropriate times.