force type `Vector{Colorant}` for `ManualDiscreteKey` #1470

dehann · 2020-08-10T15:02:41Z

Fixes an unexpected dispatch problem with a minimal code change. Seemed like the best solution after various attempts to fix downstream with user code, but didn't work. This issue also seems to be a regression since the user code has been working for quite some time.

ERROR: LoadError: LoadError: TypeError: in new, expected Array{Colorant,1}, got a value of type Array{RGB{Normed{UInt8,8}},1}
Stacktrace:
 [1] Gadfly.Guide.ManualDiscreteKey(::String, ::Array{String,1}, ::Array{Any,1}, ::Array{RGB{Normed{UInt8,8}},1}, ::Array{Function,1}, ::Array{Measures.Measure,1}, ::Bool) at /home/dehann/.julia/dev/Gadfly/src/guide/keys.jl:267
 [2] Gadfly.Guide.ManualDiscreteKey(; title::String, labels::Array{String,1}, pos::Array{Any,1}, color::Array{String,1}, shape::Array{Function,1}, size::Array{Measures.Measure,1}) at /home/dehann/.julia/dev/Gadfly/src/guide/keys.jl:295
 [3] #manual_color_key#131 at /home/dehann/.julia/dev/Gadfly/src/guide/keys.jl:321 [inlined]
 [4] manual_color_key at /home/dehann/.julia/dev/Gadfly/src/guide/keys.jl:321 [inlined]
 [5] plotKDE(::Array{BallTreeDensity,1}; c::Array{String,1}, N::Int64, rmax::Float64, rmin::Float64, axis::Nothing, dims::Nothing, xlbl::String, title::String, legend::Array{String,1}, dimLbls::Nothing, levels::Int64, fill::Bool, points::Bool, layers::Bool, overlay::Nothing) at /home/dehann/.julia/packages/KernelDensityEstimatePlotting/49JcX/src/KernelDensityEstimatePlotting.jl:356

Not sure if this is related, but:

julia> using Colors
julia> RGB isa Colorant
false
julia> RGB <: Colorant
true

Contributor checklist:

N/A I've updated the documentation to reflect these changes
I've added an entry to NEWS.md
N/A I've added and/or updated the unit tests
I've run the regression tests
I've squash'ed or fixup'ed junk commits with git-rebase
I've built the docs and confirmed these changes don't cause new errors

Proposed changes

force type Vector{Colorant}for ManualDiscreteKey in guide/keys.jl

codecov-commenter · 2020-08-10T15:38:33Z

Codecov Report

Merging #1470 into master will not change coverage.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #1470   +/-   ##
=======================================
  Coverage   89.32%   89.32%           
=======================================
  Files          39       39           
  Lines        4395     4395           
=======================================
  Hits         3926     3926           
  Misses        469      469

Impacted Files	Coverage Δ
src/guide/keys.jl	`82.69% <100.00%> (ø)`

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 d507b20...5d4d151. Read the comment docs.

Mattriks · 2020-08-10T15:49:37Z

Is this due to a type piracy issue by another package like in #1466? i.e. Can you demonstrate that the above error is caused by using Gadfly alone, without some other package being the root cause?

dehann · 2020-08-10T16:10:58Z

Hi ah, this is likely the same problem then:

Incompatibility with JuliaImage ImageCore.jl #1466,
manual_color_key fails when handed color = Colorant[RGB(...), RGB(...)] #1454
i am also loading Images.jl. So maybe best to wait for Deprecate eltype-specific convert methods JuliaImages/ImageCore.jl#131 to be merged.

This PR should be pretty harmless, since it only changes colors object type a little earlier in the stack, rather than an automated convert when new is called. I think what's happening is this PR is calling the convert inside the Gadfly context, which avoids the user code from calling the convert function in Main context, which opens the door for outside converters that don't dispatch properly to cause the error.

Mattriks · 2020-08-10T16:23:35Z

Can you give an example of your plot which uses Guide.manual_color_key. There may be a more automated way to do the plot now with recent developments on Gadfly#master (e.g. #1459)

dehann · 2020-08-10T21:38:56Z

yes, on Gadfly#master---and doesn't error without Images---but this does

using Gadfly, Images
Guide.manual_color_key("Legend", ["red"; "blue"], parse.(Colorant, ["red";"blue"]))

ERROR: TypeError: in new, expected Array{Colorant,1}, got a value of type Array{RGB{Normed{UInt8,8}},1}
Stacktrace:
 [1] ManualDiscreteKey at /home/dehann/.julia/packages/Gadfly/asb1x/src/guide/keys.jl:267 [inlined]
 [2] Gadfly.Guide.ManualDiscreteKey(; title::String, labels::Array{String,1}, pos::Array{Any,1}, color::Array{RGB{Normed{UInt8,8}},1}, shape::Array{Function,1}, size::Array{Measures.Measure,1}) at /home/dehann/.julia/packages/Gadfly/asb1x/src/guide/keys.jl:295
 [3] #manual_color_key#131 at /home/dehann/.julia/packages/Gadfly/asb1x/src/guide/keys.jl:321 [inlined]
 [4] manual_color_key(::String, ::Array{String,1}, ::Array{RGB{Normed{UInt8,8}},1}) at /home/dehann/.julia/packages/Gadfly/asb1x/src/guide/keys.jl:321
 [5] top-level scope at REPL[16]:1

EDIT, to better answer your question, the code works as follows:

function draw1D!(....; legend=nothing,....)
  # ...
  ptArr = []
  for bla in blabla
    push!(ptArr, Gadfly.layer(...))
  end
  if legend != nothing
    push!(ptArr, legend)
  end
  # ...
  return Gadfly.plot(ptArr...)
end


vecStrings = ["red";"blue"]
lg = Guide.manual_color_key("Legend", legend, parse.(Colorant, vecStrings))
H=draw1D!(....,legend=lg,....)

REF:

https://github.com/JuliaRobotics/KernelDensityEstimatePlotting.jl/blob/2b092417f0f842265ade11860c2a424bc2fe1efe/src/KernelDensityEstimatePlotting.jl#L101

dehann · 2020-08-11T04:52:24Z

An alternative suggestion here might be to force the express field type colors::Vector{Colorant} with a bespoke inner constructor that converts the type after allowing a broader incoming dispatch on ::Vector{<:Colorant}, until the type piracy in ImageCore.jl is fixed:

ManualDiscreteKey(title, labels, pos, clrs::Vector{<:Colorant}, shps, szs,  vis) = new(title, labels, pos, Vector{Colorant}(clrs), shps, szs, vis)

This might be more fitting than only 'fixing' the type inside the convenience constructor manual_color_key(..). Just a thought.

This would likely not have any performance impact, since the struct type is already ::Vector{Colorant}.

Mattriks · 2020-08-16T07:44:00Z

Thanks for the extra info about your style of plot. It is possible to automate the colorkey, and we are working to further increase support for this new style syntax. Here are some examples:

Geom.line update #1459
Geom.density2d with color grouping #1475
a Geom.density example in the plot gallery (3rd plot)
wide-format style StackOverflowError: Gadfly.draw() fails for large images #1342

Feedback welcome!

dehann · 2020-08-16T08:50:51Z

Thanks, will work through the examples you listed, and more automation on colorkey is great news! Thanks again.

Regarding the type piracy problem from ImageCore.jl, perhaps we should leave this PR open so others can find it more easily, but then close this PR when JuliaImages/ImageCore.jl#131 is merged and tagged?

dehann · 2020-09-19T21:29:22Z

Hi, I just tried this again and looks to be resolved (following recent ImageCore.jl tag). I tried with latest Gadfly#master and with updated packages. I will close this PR then.

Consequently @Mattriks , perhaps I could ask for a new tag on Gadfly.jl if possible please? We are having Pkg compat issues downstream which likely follow from this type piracy problem -- we think that a new release tag on Gadfly will help resolve the problem.

cc @Affie

EDIT: I should add that the compat issue seems to be a combination recent Optim v1.0.0 and ImageCore tags. It's a bit hard to debug, but installing Gadfly#master definitely fixes the problem. Here is an example of what the Pkg error dump looks like:

dehann added 2 commits August 10, 2020 09:59

fix dispatch, new Colorant[]

7cf1b8a

update NEWS.md

5d4d151

dehann changed the title ~~force type Vector{Colorant} in for ManualDiscreteKey~~ force type Vector{Colorant} for ManualDiscreteKey Aug 10, 2020

dehann mentioned this pull request Aug 15, 2020

fix render bug when aes.color_key_colors=nothing #1471

Closed

6 tasks

dehann mentioned this pull request Sep 18, 2020

Compat issue JuliaRobotics/RoMEPlotting.jl#123

Closed

dehann closed this Sep 19, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

force type `Vector{Colorant}` for `ManualDiscreteKey` #1470

force type `Vector{Colorant}` for `ManualDiscreteKey` #1470

dehann commented Aug 10, 2020 •

edited

Loading

codecov-commenter commented Aug 10, 2020 •

edited

Loading

Mattriks commented Aug 10, 2020

dehann commented Aug 10, 2020

Mattriks commented Aug 10, 2020

dehann commented Aug 10, 2020 •

edited

Loading

dehann commented Aug 11, 2020 •

edited

Loading

Mattriks commented Aug 16, 2020

dehann commented Aug 16, 2020

dehann commented Sep 19, 2020 •

edited

Loading

force type Vector{Colorant} for ManualDiscreteKey #1470

force type Vector{Colorant} for ManualDiscreteKey #1470

Conversation

dehann commented Aug 10, 2020 • edited Loading

Contributor checklist:

Proposed changes

codecov-commenter commented Aug 10, 2020 • edited Loading

Codecov Report

Mattriks commented Aug 10, 2020

dehann commented Aug 10, 2020

Mattriks commented Aug 10, 2020

dehann commented Aug 10, 2020 • edited Loading

dehann commented Aug 11, 2020 • edited Loading

Mattriks commented Aug 16, 2020

dehann commented Aug 16, 2020

dehann commented Sep 19, 2020 • edited Loading

force type `Vector{Colorant}` for `ManualDiscreteKey` #1470

force type `Vector{Colorant}` for `ManualDiscreteKey` #1470

dehann commented Aug 10, 2020 •

edited

Loading

codecov-commenter commented Aug 10, 2020 •

edited

Loading

dehann commented Aug 10, 2020 •

edited

Loading

dehann commented Aug 11, 2020 •

edited

Loading

dehann commented Sep 19, 2020 •

edited

Loading