Theme_colormaps

[Mattriks]

The issues behind this PR are explained here. The current implementation in Gadfly whereby Theme() replaces the whole color scale has some flaws. This PR gives Theme control of just the color map functions, so the color scales (Scale.color_discrete and Scale.color_continuous) can be used as normal. This is easier for both users and developers. E.g. the order of key colors, and transformed color scales (e.g. Scale.color_log10) now behave as expected:

using Colors, RDatasets
dpalette(n::Int) = LCHab.(65, 100, 15:(360/n):374)
cpalette = [ LCHab(30, 70, 266), LCHab(100, 0, 108), LCHab(30, 70, 12)]

mytheme = Theme(discrete_colormap= dpalette, continuous_colormap= Scale.lab_gradient(cpalette...) )

Gadfly.with_theme(mytheme) do

pa = plot(dataset("datasets","iris"),
    x=:PetalWidth, y=:SepalLength, color=:Species, Geom.point,
    Scale.color_discrete(order=[3,2,1]),
    Guide.colorkey(title="Iris", pos=[0.1, 9.3])
)

pb = plot(dataset("ggplot2","diamonds"), x=:Price, y=:Carat,
    Geom.histogram2d(xbincount=25, ybincount=25),
    Scale.color_log10,
    Scale.x_continuous(format=:plain)
)
   
hstack(pa,pb)
end   

Also I've updated the :dark theme, added a :ggplot theme, and updated the docs.

[Mattriks]

I know what the error in the test density_dark.jl is due to:

p1 = Gadfly.with_theme(:dark) do
    plot(dataset("ggplot2", "diamonds"), x=:Price, color=:Cut, Geom.density)
end
draw(PNG(4inch,3inch), p1)

Gadfly.with_theme(:dark) do
    p2 = plot(dataset("ggplot2", "diamonds"), x=:Price, color=:Cut, Geom.density)
    draw(PNG(4inch,3inch), p2)
end

Note the difference in the color scale, the first plot has reverted to the :default color mapping. Fixing it is complicated by the extra testing in density_dark.jl - to be done!

[codecov-io]

Codecov Report

Merging #1118 into master will increase coverage by 0.08%.
The diff coverage is 68.75%.

@@            Coverage Diff             @@
##           master    #1118      +/-   ##
==========================================
+ Coverage   86.12%   86.21%   +0.08%     
==========================================
  Files          32       32              
  Lines        3986     3983       -3     
==========================================
+ Hits         3433     3434       +1     
+ Misses        553      549       -4

Impacted Files	Coverage Δ
src/scale.jl	86.75% <0%> (+1.15%)	⬆️
src/Gadfly.jl	76.57% <100%> (ø)	⬆️
src/statistics.jl	93.04% <100%> (ø)	⬆️
src/theme.jl	67.79% <66.66%> (+1.75%)	⬆️
src/bincount.jl	94.84% <0%> (+1.03%)	⬆️

---

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 a111e38...454d001. Read the comment docs.

[tlnagy]

Nice! Could you add an entry to NEWS.md for this? (under v0.6.6)

[Mattriks]

I updated NEWS.md with changes for Gadfly 0.6.5 and 0.6.6.

[bjarthur]

great about NEWS.md! how about your other contributions: rect, vectorfield, smooth, etc.

[Mattriks]

Done - added those (ignoring minor fixes).

[bjarthur]

when running the regression tests (using julia test/compare_examples.jl --bw js.svg), i get different sized figures for Theme_plot_padding, and a different color scale for hexbin_dark.

[Mattriks]

There was no set_default_plot_size in Theme_plot_padding, so it was taking it from the previous testfile (subplot_scales.jl), which changed in this PR! I've added set_default_plot_size to Theme_plot_padding.

A general issue with testing named themes is that runtests.jl is calling draw outside the scope of the named theme (which would not be normal use). Hence for testing use render inside the scope (or similar functions e.g hstack, vstack).

[bjarthur]

two things:

does this PR introduce any breaking changes? if so, we'll have to wait to merge until we plan to tag a minor release.

also, a small point, but it'd be nice to use capitalization consistently in the test filenames. theme_ggplot conflicts with Theme_colormaps. i'd be inclined to use lowercase, but given that there already is a Theme_plot_padding and that types are generally capitalized in the source code, i could go either way.

[Mattriks]

Yes, this PR discontinues the use of Theme(discrete_color_scale=, continuous_color_scale=). But these are incorrectly documented in the Theme docs (I corrected the docs in #1106), so Theme(discrete_color_scale=, continuous_color_scale=) wouldn't be in common use!

Maybe Named_theme_ggplot.jl or named_theme_ggplot.jl for named theme test files?

[bjarthur]

named_theme_ggplot.jl sounds great. could you lowercase Theme_plot_padding.jl while you're at it?

would be best to deprecate the old functionality so that users are given a warning message which explains how to refactor their code. might have to keep around a method definition for Theme with the old signature and stick a call to depwarn in it.

[Mattriks]

The Theme type (and its outer function) are created by the @varset macro - is there an easy way to use depwarn in this case?

[bjarthur]

all that's really needed is a constructor function which mimics the old behavior, and converts to the new with a depwarn. don't need to redefine the old type. easiest probably just to do it by hand instead using the macro.

[Mattriks]

Currently Theme() has 55 keyword arguments (i.e. they're optional). If Theme() had e.g. 3 keyword arguments, this would be easy (as in the mock code below). So I could manually write the outer constructor for Theme() with 57 keyword arguments (2 are being deprecated, but all 57 are optional) and all their default values, in order to include a depwarn for 2 arguments which wouldn't be in common use (as explained above) - is that what I need to do? Another possibility would be to try to use a kwargs... argument, but that would only lead to recursion, I think.

module test

# this all mock code

struct ColorScale
    f::Function
end
ColorScale(;f::Function=(x,y)->x*y) = ColorScale(f)

struct Them
    a::Float64
    b::Float64
#   deprecate c::ColorScale         
    d::Function
end

# Outer constructor:
function Them(;a::Float64=0.5, b::Float64=1.5, c::T=nothing, d=(x,y)->x*y) where T<:Union{ColorScale,Void}
    
    if c!=nothing
        d = c.f
        Base.depwarn("The keyword argument `c` will be deprecated, use `d` instead", :Them)
    end
    return Them(a,b,d)
end

end

them1 = test.Them(c=test.ColorScale((x,y)->x+y)) # prints depwarn
them1.d(1,2)   # returns 3 as expected

[bjarthur]

vim macros to the rescue! people will and have complained about Gadfly breaking changes, so this is reasonably important.

default_color::ColorOrNothing=LCHab(70, 60, 240),
point_size::Measure=0.9mm,
point_size_min::Measure=0.45mm,
point_size_max::Measure=1.8mm,
point_shapes::Vector{Function}=[Shape.circle, Shape.square, Shape.diamond, Shape.cross, Shape.xcross, Shape.utriangle, Shape.dtriangle, Shape.star1, Shape.star2, Shape.hexagon, Shape.octagon, Shape.hline, Shape.vline],
line_width::Measure=0.3mm,
line_style::Union{Symbol,Vector}=:solid,
panel_fill::ColorOrNothing=nothing,
panel_stroke::ColorOrNothing=nothing,
panel_opacity::Float64=1.0,
background_color::ColorOrNothing=nothing,
plot_padding::(Vector{<:Measure})=[5mm],
grid_color::ColorOrNothing=colorant"#D0D0E0",
grid_line_style::Union{Symbol,Vector}=[0.5mm, 0.5mm],
grid_color_focused::ColorOrNothing=colorant"#A0A0A0",
grid_line_width::Measure=0.2mm,
minor_label_font::AbstractString=label_font_desc,
minor_label_font_size::Measure=8pt,
minor_label_color::ColorOrNothing=colorant"#6c606b",
major_label_font::AbstractString=title_font_desc,
major_label_font_size::Measure=11pt,
major_label_color::ColorOrNothing=colorant"#564a55",
point_label_font::AbstractString=label_font_desc,
point_label_font_size::Measure=8pt,
point_label_color::ColorOrNothing=colorant"#4c404b",
key_title_font::AbstractString=title_font_desc,
key_title_font_size::Measure=11pt,
key_title_color::ColorOrNothing=colorant"#362a35",
key_label_font::AbstractString=title_font_desc,
key_label_font_size::Measure=8pt,
key_label_color::ColorOrNothing=colorant"#4c404b",
key_color_gradations::Int=40,
bar_spacing::Measure=-0.05mm,
boxplot_spacing::Measure=1mm,
errorbar_cap_length::Measure=3mm,
stroke_color::Function=default_stroke_color,
highlight_width::Measure=0.3mm,
discrete_highlight_color::Function=default_discrete_highlight_color,
continuous_highlight_color::Function=default_continuous_highlight_color,
lowlight_color::Function=default_lowlight_color,
lowlight_opacity::Float64=0.6,
middle_color::Function=default_middle_color,
middle_width::Measure=0.6mm,
guide_title_position::Symbol=:left,
colorkey_swatch_shape::Symbol=:square,
key_position::Symbol=:right,
bar_highlight::Union{(Void), Function, Color}=nothing,
rug_size::Measure=2.0mm,
label_placement_iterations::Int=1000,
label_out_of_bounds_penalty::Float64=10.0,
label_hidden_penalty::Float64=0.5,
label_visibility_flip_pr::Float64=0.2,
key_max_columns::Int=4,
discrete_color_scale::Scale.DiscreteColorScale=Scale.color_discrete(),
continuous_color_scale::Scale.ContinuousColorScale=Scale.color_continuous(),

[bjarthur]

the merge conflicts here are going to get worse once we support 0.7. best to get this merged sooner rather than later.

[Mattriks]

This branch is a bit stale, and I think I'd prefer to close it, and I'll start a new branch sometime...

[tlnagy]

@Mattriks do you want to rebase on master and update this PR?

[tlnagy]

This would be an awesome feature to have.

[Mattriks]

I'll take another look at this after Guide-ageddon (#1156), so sometime off yet.