Plots.jl: Make Tests Pass 3.0

[daschw]

The tests are failing because of testexample 30. I just recently updated PlotReferenceImages (https://github.com/JuliaPlots/PlotReferenceImages.jl/pull/24) with Plots.test_examples(:gr, 30) looking like this:

If I run the tests locally on my linux machine (of course) the images for testexample 30 match exactly. However, Travis reports "Arrays differ. Difference: 0.10048877475329612 eps: 0.1".
So now I'm confused and wonder if something's wrong with my local setup (but all the other tests pass). It would be great if someone could check and run the tests locally. Is the picture above not how testexample 30 should look?

[massemanet]

How would one go about that? tried make test, but that is apparently too old-fashioned.

[massemanet]

Tried this julia -e 'Pkg.test("Plots"; coverage=false)', and got this;

INFO: Testing plot: gr:30:Boxplot and Violin series recipes
WARNING: Got error: ErrorException("Arrays differ.  Difference: 0.12933176475926922  eps: 0.1")
WARNING: Image did not match reference image /root/.julia/v0.6/PlotReferenceImages/Plots/gr/0.12.3/ref30.png. err: ErrorException("Arrays differ.  Difference: 0.12933176475926922  eps: 0.1")
GR: Test Failed
  Expression: image_comparison_tests(pkg, i, debug=debug, sigma=sigma, eps=eps) |> success == true
   Evaluated: false == true

Also got some ominous warnings, such as

WARNING: Gtk not loaded. err: InitError(:Gtk, ReadOnlyMemoryError())

and

WARNING: Keyword argument match_dimensions not supported with Plots.GRBackend().

and

signal (11): Segmentation fault
while loading no file, in expression starting on line 0
unknown function (ip: 0x7f63a22a8c59)
_ZN4llvm2cl19generic_parser_base10findOptionEPKc at /opt/julia/bin/../lib/julia/libLLVM-3.9.so (unknown line)

[daschw]

First of all thanks for your help!
You already cloned PlotReferenceImages.jl and have VisualRegressionTests.jl installed, right? You'd also have to have Gtk.jl, RDataSets.jl, Images.jl and ImageMagick.jl installed and StatPlots.jl and GR.jl checked out.
You can ignore the keyword argument warnings.
Could you try to run include(Pkg.dir("Plots", "test", "runtests.jl")) from the julia REPL? Then Gtk windows should open, if test_examples don't match comparing the old to the new result. I'm specifically interested in test_example 30.

[daschw]

So I just ran the test from my windows machine and got this:

The order of the different VoiceParts is rearranged. What could be the issue here?
cc: @piever

[piever]

Really not sure why this changed, but the one on the right is the correct one, with the sorted x axis.

[piever]

I thing it changed with #95, maybe @ValdarT knows?

[daschw]

The weird thing is that I just ran the tests on my linux pc locally yesterday. With linux I get the right image, with windows the left one. I have no idea, what Travis and Appveyor produce, I just know that the tests fail. Should we maybe drop StatPlots and the related examples from the Plots test suite?

[piever]

Feel free to leave out this particular test (violin and boxplot need an overhaul anyway as they have several issues). Dropping the whole StatPlots seems too much, maybe we could keep the basic stuff, like @df, plotting a distribution and groupedbar ? Those shouldn't change anytime soon and are widely used I believe.

[mkborregaard]

In fact https://github.com/JuliaPlots/StatPlots.jl/pull/95 broke violin and boxplots in other ways as well (https://github.com/JuliaPlots/StatPlots.jl/issues/121), and IMHO we should revert it and find a different fix for the case of single y values.

[ValdarT]

I thing it changed with #95, maybe @ValdarT knows?

Unfortunately, I don't. But yeah, that solution was a kind of a hack. : /

[daschw]

IMHO we should revert it and find a different fix for the case of single y values.

Agreed! Hopefully, this will fix testexample 30, otherwise I'll drop it.

Still, this issue had me thinking if maybe we should move the StatsPlots-related test_examples to a seperate test set in StatPlots (this could still use the PlotReferenceImages repo):

• Pro: Issues in StatPlots would not mess up the Plots tests.
• Con: If something fundamental changes in Plots we'd have to update ReferenceImages twice.

It's probably better to keep it as it is. Any opinions?

[daschw]

OK, I think, I figured it out. On my linux PC, where I run the tests locally, my DataFrames version was stuck at v"0.10.1" because I had ExcelReaders installed. Apparently, with DataFrames >= v"0.11.0" the image on the left in https://github.com/JuliaPlots/Plots.jl/issues/1364#issuecomment-358957076 is the correct one. So I fixed my local dependency problems and updated the testimages. The tests are passing again!

[mkborregaard]

Why is the one on the left now the correct one?

[daschw]

I have no idea. Do you think we should ask the JuliaData people, if this is intended?

[piever]

What happens is the following:

julia> using RDatasets, Query

julia> singers = RDatasets.dataset("lattice","singer")

julia> v = singers |> @map(_.VoicePart) |> collect;

julia> sort(v)
235-element Array{CategoricalArrays.CategoricalString{UInt8},1}:
 "Bass 2"   
 "Bass 2"   
 "Bass 2"   
 "Bass 2"   
 "Bass 2"   
 "Bass 2"   
 "Bass 2"   
 "Bass 2"   
 "Bass 2"   
 "Bass 2"   
 ⋮          
 "Soprano 1"
 "Soprano 1"
 "Soprano 1"
 "Soprano 1"
 "Soprano 1"
 "Soprano 1"
 "Soprano 1"
 "Soprano 1"
 "Soprano 1"

So, once it's passed through the IterableTables machinery, a CategoricalArray sorts in this funny way. Strangely enough, it sorts in the opposite order of appeareance:

julia> union(sort(v))
8-element Array{CategoricalArrays.CategoricalString{UInt8},1}:
 "Bass 2"   
 "Bass 1"   
 "Tenor 2"  
 "Tenor 1"  
 "Alto 2"   
 "Alto 1"   
 "Soprano 2"
 "Soprano 1"

julia> union(v)
8-element Array{CategoricalArrays.CategoricalString{UInt8},1}:
 "Soprano 1"
 "Soprano 2"
 "Alto 1"   
 "Alto 2"   
 "Tenor 1"  
 "Tenor 2"  
 "Bass 1"   
 "Bass 2"  

Trying to sort it before using IterableTables gives error:

julia> sort(singers[:VoicePart])
ERROR: ArgumentError: CategoricalValue objects with different pools cannot be tested for order

Definitely worth mentioning to the JuliaData people.

[mkborregaard]

Yes, that isn't the preferred behaviour IMHO.

[nalimilan]

Good catch. Sounds like a bug in IterableTables then, which ~probably re-creates a CategoricalArray by calling setindex! and without setting explicitly the ordering of levels~ EDIT: actually it just stores the CategoricalString elements in a plain Array, which isn't efficient but should normally work. It's weird that the order is reversed, though. Might be https://github.com/davidanthoff/IterableTables.jl/issues/2. Cc: @davidanthoff

EDIT: the sort error will be fixed by https://github.com/JuliaData/CategoricalArrays.jl/pull/122

[davidanthoff]

I think the iterable tables story should just pass categorical values straight through without doing anything with/to them.

I take it that the sort issues is due to something else? Is there anything I should still look into?

[nalimilan]

After investigating a bit more, it turns out that the ordering of levels is actually correct, if a bit surprising. It's exactly the same as in R, from low-pitched to high-pitched. So that's a red herring. If you want a different order, just call levels! on the CategoricalArray column.

The only problem here is that the column is turned into an Array{CategoricalString} instead of a CategoricalArray{String} by Query, which apart from being less efficient makes it impossible e.g. to call levels! on the result to change the ordering afterwards.

@davidanthoff Would it help if similar(Array, CategoricalString) or collect(CategoricalString, x) returned a CategoricalArray{String} object? Is that how you allocate the columns in Query?

[nalimilan]

I should also have noted that this means that there's no issue regarding Plots.jl AFAICT (it gives the same plot as in R). We can continue discussing semi-related issues elsewhere.

[davidanthoff]

@nalimilan It is really up to each individual sink what it does when it encounters a CategoricalString or one of those types.

I believe StatPlots.jl is using TableTraitsUtils.jl to help with that. One can pass a helper function to the method that is used there that specifies what array type should be used for a given element type. So I think it might be enough if in this line here one would pass a different array factory function than the default one that would create a CategoricalArray{String} whenever it encounters a CategoricalString.

[nalimilan]

OK, thanks. But the example above did no involve StatPlots at all, so I'm not sure what can be done about it. It sounds like we need a generic API to create the best AbstractArray object for a given type and dimensions, with a fallback on Array. For example, it could be similar(AbstractArray, T, n) (see https://github.com/JuliaLang/julia/pull/20815). Then it would automatically work for any custom types like CategoricalArray without any special case

[mkborregaard]

I'm a little confused whether we need to do anything in StatPlots to deal with this?

[nalimilan]

I'm not sure. The plot in the OP (with groups in the alphabetical order) is incorrect AFAICT, so if it's the one which is currently produced then there's a bug. OTC, the LHS plot at https://github.com/JuliaPlots/Plots.jl/issues/1364#issuecomment-358957076 looks correct, so if you get that I think everything is fine.

Then there's a possible question about whether StatPlots produces Vector{CategoricalString} objects, which should rather be CategoricalVector{String} for performance, but that's a more general problem and for plotting it's probably not a big deal anyway.

[mkborregaard]

Cool, thanks a lot.

[davidanthoff]

I reread the thread, and all of this has actually nothing to do with iterable tables. The query with the @map that @piever showed above returns an iterator with an element type of CategoricalString. The collect method that gets called on that is from base. So if we want this to result in a CategoricalVector, we would just need a way to hook into base collect that allows CategoricalArrays.jl to register a special array type to be used for iterators of CategoricalString elements. Such a hook would be fantastic, I’ve suggested this kind of array factory pattern before, because it would also allow me to make the DataValueArray story as smooth as the Array{Union{T,Missing}} story.

[nalimilan]

Isn't this what I asked above, i.e. would it help if CategoricalArrays overrode collect(::Array{<:CategoricalString}) so that it returns a CategoricalArray?