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

Time-to-first-plot improvements #1278

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

Time-to-first-plot improvements #1278

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
0f11ad0
Remove unnecessary splatting from internal interfaces
non-Jedi Apr 18, 2019
bf5bd2e
Compare nothing by type rather than by value
non-Jedi Apr 19, 2019
80ae3df
Compare nothing values with ===
non-Jedi May 8, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion Project.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ StatsBase = "2913bbd2-ae8a-5f71-8c99-4fb6c76f3a91"
[compat]
CategoricalArrays = "0.5"
Colors = "0.9"
Compat = "2"
Compat = "2.1"
Compose = "0.7"
Contour = "0.5"
CoupledFields = "0.1"
Expand Down
61 changes: 30 additions & 31 deletions src/Gadfly.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -388,14 +388,14 @@ function render_prepare(plot::Plot)
# they are missing.
datas = Array{Data}(undef, length(plot.layers))
for (i, layer) in enumerate(plot.layers)
if layer.data_source === nothing && isempty(layer.mapping)
if isnothing(layer.data_source) && isempty(layer.mapping)
Copy link

Choose a reason for hiding this comment

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

I'd be willing to bet === nothing is more efficient. The compiler loves ===.

Copy link

Choose a reason for hiding this comment

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

Simply changing occurrences of == nothing to use === (and likewise for the negative) should provide a nice improvement.

Copy link

Choose a reason for hiding this comment

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

For reference, this issue applies: JuliaLang/julia#27681

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good to know. I'll update with straight === and see if it makes a difference.

Copy link

Choose a reason for hiding this comment

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

That would also resolve the isnothing name qualification discussion.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've pushed a commit changing from isnothing to === nothing. Still need to get back to the computer I've benchmarked on before to see if it makes a difference.

layer.data_source = plot.data_source
layer.mapping = plot.mapping
datas[i] = plot.data
else
datas[i] = Data()

if layer.data_source === nothing
if isnothing(layer.data_source)
layer.data_source = plot.data_source
end

Expand All @@ -416,7 +416,7 @@ function render_prepare(plot::Plot)
if isa(layer.geom, Geom.SubplotGeometry)
for subplot_layer in layers(layer.geom)
subplot_data = Data()
if subplot_layer.data_source === nothing
if isnothing(subplot_layer.data_source)
subplot_layer.data_source = layer.data_source
end

Expand All @@ -434,7 +434,7 @@ function render_prepare(plot::Plot)
coord = plot.coord
for layer in plot.layers
coord_type = element_coordinate_type(layer.geom)
if coord === nothing
if isnothing(coord)
coord = coord_type()
elseif typeof(coord) != coord_type
error("Plot uses multiple coordinates: $(typeof(coord)) and $(coord_type)")
Expand Down Expand Up @@ -503,7 +503,7 @@ function render_prepare(plot::Plot)

unscaled_aesthetics = setdiff(used_aesthetics, scaled_aesthetics)

_theme(plt, lyr) = lyr.theme == nothing ? plt.theme : lyr.theme
_theme(plt, lyr) = isnothing(lyr.theme) ? plt.theme : lyr.theme

# Add default scales for statistics.
layer_stats_with_theme = map(plot.layers, layer_stats) do l, stats
Expand Down Expand Up @@ -532,17 +532,17 @@ function render_prepare(plot::Plot)
in(var, mapped_aesthetics) || continue

var_data = getfield(plot.data, var)
if var_data == nothing
if isnothing(var_data)
for data in datas
var_layer_data = getfield(data, var)
if var_layer_data != nothing
if !isnothing(var_layer_data)
var_data = var_layer_data
break
end
end
end

var_data == nothing && continue
isnothing(var_data) && continue

t = classify_data(var_data)
if scale_exists(t, var)
Expand All @@ -560,7 +560,7 @@ function render_prepare(plot::Plot)
t = :categorical
for data in Iterators.flatten((datas, subplot_datas))
val = getfield(data, var)
if val != nothing && val != :categorical
if !isnothing(val) && val != :categorical
t = classify_data(val)
end
end
Expand Down Expand Up @@ -653,30 +653,33 @@ function render_prepare(plot::Plot)

# I. Scales
layer_aess = Scale.apply_scales(IterTools.distinct(values(scales)),
datas..., subplot_datas...)
vcat(datas, subplot_datas))

# set defaults for key titles
keyvars = [:color, :shape]
for (i, layer) in enumerate(plot.layers)
for kv in keyvars
fflag = (getfield(layer_aess[i], Symbol(kv,"_key_title")) == nothing) && haskey(layer.mapping, kv) && !isa(layer.mapping[kv], AbstractArray)
fflag = (isnothing(getfield(layer_aess[i], Symbol(kv,"_key_title")))) &&
haskey(layer.mapping, kv) &&
!isa(layer.mapping[kv], AbstractArray)
fflag && setfield!(layer_aess[i], Symbol(kv,"_key_title"), string(layer.mapping[kv]))
end
end

for kv in keyvars
fflag = (getfield(layer_aess[1], Symbol(kv,"_key_title")) == nothing) && haskey(plot.mapping, kv) && !isa(plot.mapping[kv], AbstractArray)
fflag = (isnothing(getfield(layer_aess[1], Symbol(kv,"_key_title")))) &&
haskey(plot.mapping, kv) && !isa(plot.mapping[kv], AbstractArray)
fflag && setfield!(layer_aess[1], Symbol(kv,"_key_title"), string(plot.mapping[kv]))
end

# Auto-update color scale if shape==color
catdatas = vcat(datas, subplot_datas)
shapev = getfield.(catdatas, :shape)
di = (shapev.!=nothing) .& (shapev.== getfield.(catdatas, :color))
di = (!isnothing).(shapev) .& (shapev.== getfield.(catdatas, :color))

supress_colorkey = false
for (aes, data) in zip(layer_aess[di], catdatas[di])
aes.shape_key_title==nothing && (aes.shape_key_title=aes.color_key_title="Shape")
isnothing(aes.shape_key_title) && (aes.shape_key_title=aes.color_key_title="Shape")
colorf = scales[:color].f
scales[:color] = Scale.color_discrete(colorf, levels=scales[:shape].levels, order=scales[:shape].order)
Scale.apply_scale(scales[:color], [aes], Gadfly.Data(color=getfield(data,:color)) )
Expand All @@ -692,7 +695,7 @@ function render_prepare(plot::Plot)
end

# IIb. Plot-wise Statistics
plot_aes = concat(layer_aess...)
plot_aes = concat(layer_aess)
statistics = collect(statistics)
Stat.apply_statistics(statistics, scales, coord, plot_aes)

Expand All @@ -713,7 +716,7 @@ function render_prepare(plot::Plot)

if !supress_keys
for (KT, kv) in zip(keytypes, keyvars)
fflag = !all([getfield(aes, kv)==nothing for aes in [plot_aes, layer_aess...]])
fflag = !all([isnothing(getfield(aes, kv)) for aes in [plot_aes, layer_aess...]])
fflag && !in(KT, explicit_guide_types) && push!(guides, KT())
end
end
Expand Down Expand Up @@ -756,7 +759,7 @@ function render(plot::Plot)

ctx = pad_inner(root_context, plot.theme.plot_padding...)

if plot.theme.background_color != nothing
if !isnothing(plot.theme.background_color)
compose!(ctx, (context(order=-1000000),
fill(plot.theme.background_color),
stroke(nothing), rectangle()))
Expand Down Expand Up @@ -807,30 +810,26 @@ function render_prepared(plot::Plot,
layer_aess), scales)

# IV. Geometries
themes = Theme[layer.theme === nothing ? plot.theme : layer.theme
themes = Theme[isnothing(layer.theme) ? plot.theme : layer.theme
for layer in plot.layers]
zips = trim_zip(plot.layers, layer_aess,
layer_subplot_aess,
layer_subplot_datas,
themes)

compose!(plot_context,
[compose(context(order=layer.order), render(layer.geom, theme, aes,
subplot_aes, subplot_data,
scales))
for (layer, aes, subplot_aes, subplot_data, theme) in zips]...)
zips = trim_zip(plot.layers, layer_aess, layer_subplot_aess,
layer_subplot_datas, themes)
for (layer, aes, subplot_aes, subplot_data, theme) in zips
r = render(layer.geom, theme, aes, subplot_aes, subplot_data, scales)
compose!(plot_context, (context(order=layer.order), r))
end#for

# V. Guides
guide_contexts = Any[]
guide_contexts = Guide.PositionedGuide[]
for guide in guides
guide_context = render(guide, plot.theme, plot_aes, dynamic)
if guide_context != nothing
if !isnothing(guide_context)
append!(guide_contexts, guide_context)
end
end

tbl = Guide.layout_guides(plot_context, coord,
plot.theme, guide_contexts...)
plot.theme, guide_contexts)
if table_only
return tbl
end
Expand Down
57 changes: 26 additions & 31 deletions src/aesthetics.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,7 +102,7 @@ function show(io::IO, data::Aesthetics)
print(io, "Aesthetics(")
for name in valid_aesthetics
val = getfield(data, name)
if !ismissing(val) && val != nothing
if !ismissing(val) && !isnothing(val)
print(io, "\n ", string(name), "=")
show(io, getfield(data, name))
end
Expand Down Expand Up @@ -140,7 +140,7 @@ getindex(aes::Aesthetics, i::Integer, j::AbstractString) = getfield(aes, Symbol(
function defined_aesthetics(aes::Aesthetics)
vars = Set{Symbol}()
for name in valid_aesthetics
getfield(aes, name) === nothing || push!(vars, name)
isnothing(getfield(aes, name)) || push!(vars, name)
end
vars
end
Expand Down Expand Up @@ -172,7 +172,7 @@ function assert_aesthetics_undefined(who::AbstractString, aes::Aesthetics, vars:
end

function assert_aesthetics_equal_length(who::AbstractString, aes::Aesthetics, vars::Symbol...)
defined_vars = Compat.Iterators.filter(var -> !(getfield(aes, var) === nothing), vars)
defined_vars = Compat.Iterators.filter(var -> !isnothing(getfield(aes, var)), vars)

if !isempty(defined_vars)
n = length(getfield(aes, first(defined_vars)))
Expand All @@ -198,7 +198,7 @@ end
#
function update!(a::Aesthetics, b::Aesthetics)
for name in valid_aesthetics
issomething(getfield(b, name)) && setfield(a, name, getfield(b, name))
!isnothing(getfield(b, name)) && setfield(a, name, getfield(b, name))
end
nothing
end
Expand Down Expand Up @@ -226,26 +226,21 @@ json(a::Aesthetics) = join([string(a, ":", json(getfield(a, var))) for var in ae
# Returns:
# A new Aesthetics instance with vectors concatenated.
#
function concat(aess::Aesthetics...)
function concat(aess)
cataes = Aesthetics()
for aes in aess
for var in valid_aesthetics
if var in [:xviewmin, :yviewmin]
mu, mv = getfield(cataes, var), getfield(aes, var)
setfield!(cataes, var,
mu === nothing ? mv :
mv == nothing ? mu :
min(mu, mv))
elseif var in [:xviewmax, :yviewmax]
mu, mv = getfield(cataes, var), getfield(aes, var)
setfield!(cataes, var,
mu === nothing ? mv :
mv == nothing ? mu :
max(mu, mv))
mu, mv = getfield(cataes, var), getfield(aes, var)
isviewmin = var in (:xviewmin, :yviewmin)
isviewmax = var in (:xviewmax, :yviewmax)
mumv = if isviewmin
isnothing(mu) ? mv : isnothing(mv) ? mu : min(mu, mv)
elseif isviewmax
isnothing(mu) ? mv : isnothing(mv) ? mu : max(mu, mv)
else
setfield!(cataes, var,
cat_aes_var!(getfield(cataes, var), getfield(aes, var)))
end
cat_aes_var!(mu, mv)
end#if
setfield!(cataes, var, mumv)
end
end
cataes
Expand Down Expand Up @@ -313,21 +308,21 @@ end
#
function by_xy_group(aes::T, xgroup, ygroup,
num_xgroups, num_ygroups) where T <: Union{Data, Aesthetics}
@assert xgroup === nothing || ygroup === nothing || length(xgroup) == length(ygroup)
@assert isnothing(xgroup) || isnothing(ygroup) || length(xgroup) == length(ygroup)

n = num_ygroups
m = num_xgroups

xrefs = xgroup === nothing ? [1] : xgroup
yrefs = ygroup === nothing ? [1] : ygroup
xrefs = isnothing(xgroup) ? [1] : xgroup
yrefs = isnothing(ygroup) ? [1] : ygroup

aes_grid = Array{T}(undef, n, m)
staging = Array{AbstractArray}(undef, n, m)
for i in 1:n, j in 1:m
aes_grid[i, j] = T()
end

xgroup === nothing && ygroup === nothing && return aes_grid
isnothing(xgroup) && isnothing(ygroup) && return aes_grid

function make_pooled_array(::Type{IndirectArray{T,N,A,V}}, arr::AbstractArray) where {T,N,A,V}
uarr = unique(arr)
Expand All @@ -350,8 +345,8 @@ function by_xy_group(aes::T, xgroup, ygroup,

vals = getfield(aes, var)
if typeof(vals) <: AbstractArray
if xgroup !== nothing && length(vals) !== length(xgroup) ||
ygroup !== nothing && length(vals) !== length(ygroup)
if !isnothing(xgroup) && length(vals) !== length(xgroup) ||
!isnothing(ygroup) && length(vals) !== length(ygroup)
continue
end

Expand Down Expand Up @@ -405,12 +400,12 @@ function inherit!(a::Aesthetics, b::Aesthetics;
bval = getfield(b, field)
if field in clobber_set
setfield!(a, field, bval)
elseif aval === missing || aval === nothing || aval === string || aval == showoff
elseif aval === missing || isnothing(aval) || aval === string || aval === showoff
setfield!(a, field, bval)
elseif field == :xviewmin || field == :yviewmin
bval != nothing && (aval == nothing || aval > bval) && setfield!(a, field, bval)
elseif field == :xviewmax || field == :yviewmax
bval != nothing && (aval == nothing || aval < bval) && setfield!(a, field, bval)
elseif field in (:xviewmin, :yviewmin) && !isnothing(bval)
(isnothing(aval) || aval > bval) && setfield!(a, field, bval)
elseif field in (:xviewmax, :yviewmax) && !isnothing(bval)
(isnothing(aval) || aval < bval) && setfield!(a, field, bval)
elseif typeof(aval) <: Dict && typeof(bval) <: Dict
merge!(aval, getfield(b, field))
end
Expand Down