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

enable image display using ImageShow #41

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Conversation

johnnychen94
Copy link
Member

@johnnychen94 johnnychen94 commented Dec 14, 2019

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
Copy link
Member Author

johnnychen94 commented Dec 14, 2019

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
Copy link

coveralls commented Dec 14, 2019

Coverage Status

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

@coveralls
Copy link

Coverage Status

Coverage decreased (-1.1%) to 81.183% when pulling 5540741 on jc/display into 46d650a on master.

@johnnychen94
Copy link
Member Author

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
Copy link
Member Author

johnnychen94 commented Dec 16, 2019

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

There're two paths in front:

  • By default, render images as general arrays, and use Requires.jl to make ImageInTerminal, ImageShow and possibly Plots as possible plugins for advanced render experience. These could enable advanced render strategy for images while keeping the dependency not blowing up.
  • Make ImageInTerminal and ImageShow(lightweight as well) as dependencies to provide basic image render experience, while using Requires or simply making another package for advanced and also heavy ploting packages.

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
Copy link
Member

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
Copy link
Member Author

johnnychen94 commented Dec 18, 2019

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

  • Without ImageInTerminal or ImageShow: WIP
  • with ImageInTerminal (Current status): 5.517030 seconds (10.58 M allocations: 557.445 MiB, 3.22% gc time)
  • with ImageInTerminal and ImageShow (This PR): 7.162142 seconds (13.72 M allocations: 718.257 MiB, 3.03% gc time)

Just proposed to make FFTW a soft dependency of ImageCore JuliaImages/ImageCore.jl#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
Copy link
Member

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
Copy link
Member Author

johnnychen94 commented Dec 19, 2019

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

@oxinabox
Copy link
Member

oxinabox commented Dec 19, 2019

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

@johnnychen94
Copy link
Member Author

johnnychen94 commented Jan 2, 2020

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):

  • ImageCore v0.8.10: 7.498385 seconds (10.22 M allocations: 542.104 MiB, 3.40% gc time)
  • ImageCore v0.8.7: 8.284098 seconds (10.91 M allocations: 581.098 MiB, 3.27% gc time)
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

src/render.jl Outdated
@@ -24,13 +24,27 @@ end

## 2 arg form render for comparing
function render(mode::BeforeAfter, reference, actual)
if showable(MIME("image/png"), actual)
Copy link
Member Author

@johnnychen94 johnnychen94 Mar 14, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This if statement is always true if ImageShow is loaded, but what I actually want to check is if there's an available display for it.

Do you know how could I achieve it? @oxinabox

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think displayable instread of showable

println("- REFERENCE -------------------")
render_item(mode, reference)
println("-------------------------------")
println("- ACTUAL ----------------------")
render_item(mode, actual)
println("-------------------------------")
end
function render(::MIME"image/png", mode::BeforeAfterImage, reference, actual)
println("- REFERENCE --------|--------- ACTUAL -")
display(MIME("image/png"), mosaicview(reference, actual; nrow=1, npad=5))
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought MIME("image/png") or "image/png" would work, but this still renders images both in atom plot panel and in terminal 😕

@@ -19,6 +21,8 @@ Distances = "0.7, 0.8"
FileIO = "1"
ImageCore = "0.8.1"
ImageInTerminal = "0.3, 0.4"
ImageShow = "0.2"
MosaicViews = "0.2"
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MosaicView is planned to be reexported by ImageShow

JuliaImages/ImageShow.jl#19

@johnnychen94
Copy link
Member Author

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
Copy link
Member

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.

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.

3 participants