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.

Sign up for GitHub

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

Jump to bottom

Add @test_reference_plot #76

Closed
wants to merge 1 commit into from
Closed

Add @test_reference_plot #76

wants to merge 1 commit into from

Conversation

juliohm
Copy link
Contributor

@juliohm juliohm commented Feb 10, 2021

Fix #62

I need some help with the keyword options. I tried to pass them to the inner @test_reference call but got errors and couldn't fix them myself.

@juliohm
Copy link
Contributor Author

juliohm commented Feb 10, 2021

If you would like, I can also move to the new Pkg approach with a separate test/Project.toml file. It is cleaner than this [extras] section.

@johnnychen94
Copy link
Member

johnnychen94 commented Feb 11, 2021 •
edited
Loading

I think a better approach is to specialize _convert method for Plot types(AbstractPlot, maybe. I don't know), and use Requires.jl to conditionally load that specialization method when Plots.jl is loaded.

Also, it doesn't sound good to me to drop 1.0 support only to give Plots.jl support, which means any further API changes we planed in #58 will not be available in Julia 1.0. @oxinabox how do you think?

FWIW, I believe both PNGFiles(requires Julia >1.3) and ImageMagick(compatible to "all" Julia versions) allows an IO stream pipeline.

If you would like, I can also move to the new Pkg approach with a separate test/Project.toml file. It is cleaner than this [extras] section.

Please don't do this in this PR, let's keep this PR as simple as it can. Also do note that using a separate test/Project.toml requires Julia >= 1.2.

@juliohm
Copy link
Contributor Author

juliohm commented Feb 11, 2021 via email

@johnnychen94
Copy link
Member

Why should we introduce a new API when the current test_reference pipeline already enables you to extend it?

@juliohm
Copy link
Contributor Author

juliohm commented Feb 11, 2021 via email

@johnnychen94
Copy link
Member

I would be happier to have a clean design with a relatively small Requires.jl dependency; many packages(including Plots) load Requires so it won't be a big deal.

@juliohm
Copy link
Contributor Author

juliohm commented Feb 11, 2021 via email

@johnnychen94
Copy link
Member

We only need the type AbstractPlot and I don't think it will be changed in a foreseeable future. As I've said, the internal pipeline design already enables you to extend the support to Plots.

Also, the responsibility of a maintainer is to merge and fix upstream breaking changes, and not introduce breaking changes to downstream. We already have plans to deprecate the whole test_reference macro in favor of match_reference function, so adding a API that must be deprecated in short future is not a good option, especially when it's exported.

What's more, even if we need to introduce a new function, it should be asimage only. The new marco wrapper is definitely unnecessary.

To be more clear, I'd strongly reject this version if there's no other reviewer involved. These codes don't really need to exist in this package, it can be put in any package that you use. And it's totally acceptable to have some duplication for test only codes.

@juliohm
Copy link
Contributor Author

juliohm commented Feb 11, 2021 via email

@juliohm
Copy link
Contributor Author

juliohm commented Feb 11, 2021

Just to reiterate my point of view here @johnnychen94 , I think you need to distinguish between what is an elegant design and what is a practical solution that addresses user needs. If you only aim for the first goal, you will likely spend more time than needed developing the package. If the design is too complex to maintain, only a few people will be aware of the dispatch rules and internal behavior, which will also limit future fixes by end users. Overdesign can be a problem as you know.

I tend to think of this effort here as a centralized development of a general isapprox for different objects. Whether this will lead to separate @test_img @test_plot @test_array that are easy to understand or to a single complex match_reference, that is up to the main developers, and also a function of how much time you are willing to spend evolving the code base when someone reports an issue. To me this package solves a very specific problem and doesn't need a sophisticated design. However, if you feel that it could become part of Julia Base or something bigger, then it makes sense to carefully evolve the design.

@oxinabox
Copy link
Member

oxinabox commented Feb 11, 2021 •
edited
Loading

I am strongly against adding a Plots dependency, even behind Requires.

  1. Plots adds a ton to load time, and causes weird errors on some CI systems unless you do a thing. Putting it behind Requires.jl does solve that.
  2. Requires.jl significantly increases load-time, if you do need to load the package.
  3. As mentioned Required.jl doesn't play nice with compat, while it seems safe to assume that Plots.jl won't have breaking changes to the existance of AbstractPlot, I still think its not worth it. We have enough similar weirdness to that from using FileIO

This is a nice solution because it doesn't care about Plots at all, just showable.
calling it test_reference_plot is a bit of a minomer.

I think we might be able to do this without even adding a new command; or adding any new dependencies.
I note that:

using Plots
using ImageIO  # loaded via FileIO.jl
using ReferenceTests

 @test_reference "temp.png" heatmap([1 0; 0 1])

will the first time it is called create the actual PNG that is wanted on disk.

Then calling it again (so it has a PNG top load)
errors here:

ReferenceTests.jl/src/fileio.jl

Lines 7 to 9 in 31deee3

function loadfile(::Type{T}, file::File) where T
T(load(file))::T # Fallback to FileIO
end

With

ERROR: MethodError: no method matching Plots.Plot{Plots.GRBackend}(::Matrix{RGB{FixedPointNumbers.N0f8}})
Closest candidates are:
 Plots.Plot{T}(::Any, ::Any, ::Any, ::Any, ::Any, ::Any, ::Any, ::Any, ::Any, ::Any) where T<:AbstractBackend at /Users/oxinabox/.julia/packages/Plots/6EMd6/src/types.jl:68
Stacktrace:

Note that the type is Matrix{RGB{FixedPointNumbers.N0f8}}
which AIUI is how a correctly loaded image is represented in the Images.jl ecosystem.
The error message is that you can;t call Plots.Plot{Plots.GRBackend} on it.
But actually you don't need to because it is already in the right form?

So maybe all we need to do is remove that T? maybe it is just a mistake to have it there?

function loadfile(::Type{T}, file::File) where T
   T(load(file))::T # Fallback to FileIO
end

Or maybe we need to have a fallback:

function loadfile(::Type{T}, file::File) where T
    # Fallback to FileIO
    data = load(file)
    try
        T(data)
    catch
        data
    end
end

I don't have time to investigate more right now.
But it seems very promising that the temp.png file is being created (and almost loaded) correctly.

I think a better approach is to specialize _convert method for Plot types(AbstractPlot, maybe. I don't know), and use Requires.jl to conditionally load that specialization method when Plots.jl is loaded.

We might need to overload _convert but not to specialize on AbstractPlot, but rather on: ::Type{DataFormat{:PNG}}

Rather than adding dependency on PNGFiles at all
we might just want to write to a tempfile and read it back in.

function _convert(::Type{<:DataFomat{:PNG}}, data}
    mktempdir() do dir
        filename = File{DataFomat{:PNG}}(joinpath(dir, "inconversion.png"))
        savefile(filename, data)
        load(filename)
    end
end

Tempfiles often go to a RAM Disk anyway

Please don't do this in this PR, let's keep this PR as simple as it can. Also do note that using a separate test/Project.toml requires Julia >= 1.2.

I agree. Dropping 1.0 support would be bad.

@juliohm
Copy link
Contributor Author

juliohm commented Feb 15, 2021

It is not clear to me what could be a temporary solution to this issue. Can you please advise on how I could update the PR so that it gets merged and so that we don't need to copy the asimage everywhere in our packages? I have at least 10 packages that are now using ReferenceTests.jl

@oxinabox
Copy link
Member

oxinabox commented Feb 15, 2021 •
edited
Loading

update the PR to make @test_reference do this without needing a new macro with a different name.
This can be done with appropriate changes to loadfile and _convert so that they can handle thing with DataFormat{:PNG} and generic object types (which will include AbstractPlot).
and avoid adding a PNGFiles dependancy by writing to a tempfile instead.

@oxinabox oxinabox mentioned this pull request Feb 15, 2021
Merged
@oxinabox
Copy link
Member

Closing given #77

@oxinabox oxinabox closed this Feb 20, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

@plottest
3 participants
@juliohm @johnnychen94 @oxinabox