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.

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

support julia 1.0 #1189

Merged
merged 11 commits into from
Sep 14, 2018
Merged

support julia 1.0 #1189

merged 11 commits into from
Sep 14, 2018

Conversation

bjarthur
Copy link
Member

fixes #1158, #1185

@bjarthur
Copy link
Member Author

still a WIP...

@bjarthur
Copy link
Member Author

bjarthur commented Aug 26, 2018

anyone have suggestions about how to perform regression testing with the change in the random number generator in 0.7? am i correct in thinking the old sequence of random numbers is not reproducible? if so, then one thing we could do is replace calls to rand in the test scripts with hard-coded random numbers. doing so is feasible for sequences less than say 20 in length, which is about half of them.

@bjarthur
Copy link
Member Author

https://github.com/danspielman/RandomV06.jl should make the regression testing easy.

@bjarthur
Copy link
Member Author

should we tag a 0.6 release while we finish support for 0.7? that would give folks some time to make sure nothing was broken on what would hopefully be the last 0.6 tag before we merge this 0.7 PR.

@@ -46,7 +46,7 @@ function polygon_points(xs, ys, preserve_order)
return T[(x, y) for (x, y) in zip(xs, ys)]
else
centroid_x, centroid_y = mean(xs), mean(ys)
θ = atan2(xs - centroid_x, ys - centroid_y)
θ = atan(xs - centroid_x, ys - centroid_y)
Copy link
Member

Choose a reason for hiding this comment

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

atan.() because xs and ys are vectors here.

@bjarthur bjarthur force-pushed the bja/julia07 branch 2 times, most recently from 10a42c9 to fe997d1 Compare August 31, 2018 12:11
REQUIRE Outdated
@@ -14,8 +14,9 @@ IterTools
JSON
KernelDensity
Loess
Measures
REPL

Choose a reason for hiding this comment

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

Stdlib packages shouldn't go in REQUIRE.

@@ -370,7 +370,7 @@ function by_xy_group(aes::T, xgroup, ygroup,
if !applicable(convert, typeof(vals), staging[i, j])
T2 = eltype(vals)
if T2 <: Color T2 = Color end
da = DataArray(T2, length(staging[i, j]))
da = Array{Union{Missing,T2}}(length(staging[i, j]))

Choose a reason for hiding this comment

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

This will also need an undef, or you can initialize with missings.

Copy link
Member Author

Choose a reason for hiding this comment

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

why didn't femtocleaner catch this?

Copy link

Choose a reason for hiding this comment

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

I've actually seen quite a few instances of FemtoCleaner not getting the undef thing.

Copy link
Member Author

Choose a reason for hiding this comment

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

ditto. is it worth submitting an issue to femtocleaner?

Copy link

Choose a reason for hiding this comment

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

Could be, yeah.

src/coord.jl Outdated
for aes in aess
if !isnull(aes.pad_categorical_x)
if isnull(pad_categorical_x)
if aes.pad_categorical_x!==missing

Choose a reason for hiding this comment

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

It would be nice to put space around !== (and === for symmetry) because it's unclear at first glance whether it should be e.g. x! == y or x !== y.

src/misc.jl Outdated
mapping = coalesce.(indexin(CategoricalArrays.index(values.pool), levels), 0)
unshift!(mapping, coalesce(findfirst(ismissing, levels), 0))
mapping = Union{Nothing,Int}[coalesce.(indexin(CategoricalArrays.index(values.pool), levels), 0)...]
pushfirst!(mapping, coalesce(findfirst(ismissing, levels), 0))

Choose a reason for hiding this comment

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

The coalesce here should be something, since findfirst will return nothing rather than missing. The same likely applies above. (Also indexin might be deprecated, can't quite recall.)

Copy link
Contributor

Choose a reason for hiding this comment

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

Also AFAICT the comprehension isn't necessary.

Copy link
Contributor

Choose a reason for hiding this comment

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

@bjarthur something should be used rather than coalesce as @ararslan noted. The current code does not replace nothing with 0 as it should, so I'm not even sure how it works.

Regarding the use of the comprehension, what bothers me is that you're splatting a potentially very large array, which will force recompiling a specialized function for each length. What error do you get without it?

@tlnagy
Copy link
Member

tlnagy commented Sep 3, 2018

should we tag a 0.6 release while we finish support for 0.7? that would give folks some time to make sure nothing was broken on what would hopefully be the last 0.6 tag before we merge this 0.7 PR.

Most definitely.

@bjarthur
Copy link
Member Author

bjarthur commented Sep 3, 2018

gadfly v0.8.0 tagged and merged. hopefully the last version to suport julia 0.6.
JuliaLang/METADATA.jl#17578

@ararslan
Copy link

ararslan commented Sep 3, 2018

I'd recommend these changes here:

diff --git a/.travis.yml b/.travis.yml
index 5b1789f..4b90535 100644
--- a/.travis.yml
+++ b/.travis.yml
@@ -2,8 +2,8 @@ language: julia
 os:
   - linux
 julia:
-  - 0.6
   - 0.7
+  - 1.0
   - nightly
 matrix:
   fast_finish: true
@@ -11,12 +11,15 @@ matrix:
     - julia: nightly
 notifications:
   email: false
-script:
-  - if [[ -a .git/shallow ]]; then git fetch --unshallow; fi
-  - julia -e 'Pkg.clone(pwd()); Pkg.build("Gadfly");'
-  - julia -e 'Pkg.checkout("Compose")'
-  - julia --check-bounds=yes -e 'Pkg.test("Gadfly", coverage=true)'
+before_script:
+  - julia -e 'using Pkg; Pkg.add(PackageSpec(name="Compose", rev="master"))'
 after_success:
-  - julia -e 'cd(Pkg.dir("Gadfly")); Pkg.add("Coverage"); using Coverage; Codecov.submit(process_folder())'
-  - julia -e 'Pkg.checkout("Compose")'
-  - if [[ "$TRAVIS_JULIA_VERSION" == "0.6" ]]; then julia -e 'cd(Pkg.dir("Gadfly")); map(x->Pkg.add(strip(x)), readlines(open(joinpath("docs", "REQUIRE")))); include(joinpath("docs", "make.jl"))'; fi
+  - julia -e 'using Pkg; Pkg.add("Coverage"); using Coverage; Codecov.submit(process_folder())'
+  - |
+      julia -e '
+          using Pkg
+          Pkg.add("Documenter")
+          # These packages need to be installed explicitly since they are used by name in the docs
+          foreach(Pkg.add, ["RDatasets", "Distributions", "StatsBase", "Colors", "DataFrames", "Compose"])
+          include(joinpath("docs", "make.jl"))
+      '
diff --git a/docs/make.jl b/docs/make.jl
index d670fe1..52c45ff 100644
--- a/docs/make.jl
+++ b/docs/make.jl
@@ -41,7 +41,7 @@ makedocs(
 
 deploydocs(
     repo   = "github.com/GiovineItalia/Gadfly.jl.git",
-    julia  = "0.6",
+    julia  = "1.0",
     osname = "linux",
     deps = nothing,
     make = nothing,

@bjarthur
Copy link
Member Author

bjarthur commented Sep 3, 2018

anyone know how to get a line number out of the following warning?

┌ Warning: broadcast will default to iterating over its arguments in the future. Wrap arguments of
│ type `x::Dict{Type,Gadfly.GuideElement}` with `Ref(x)` to ensure they broadcast as "scalar" elements.
│   caller = ip:0x0
└ @ Core :-1

@bjarthur bjarthur force-pushed the bja/julia07 branch 3 times, most recently from 7959183 to 3b5b490 Compare September 7, 2018 11:45
@tlnagy tlnagy mentioned this pull request Sep 7, 2018
@tlnagy
Copy link
Member

tlnagy commented Sep 7, 2018

On Julia 1.0 with Compose master and @rsrock's Hexagons PR (GiovineItalia/Hexagons.jl#10). Gadfly loads, but I get the following

WARNING: could not import Base.start into Gadfly
WARNING: could not import Base.next into Gadfly
WARNING: could not import Base.done into Gadfly
WARNING: eval from module Media to Gadfly:    
Expr(:where, Expr(:call, Expr(:., :Media, :(:media)), Expr(:::, Expr(:curly, :Type, :T))), Expr(:<:, :T, Gadfly.Plot)) = Expr(:block, #= Symbol("/home/tamas/.julia/packages/Media/Lrdeg/src/system.jl"):71 =#, Expr(:ref, Array{Any, (1,)}[Media.Plot], 1))
  ** incremental compilation may be broken for this module **

Also, tests crash with

[ Info: Guide_colorkey.pdf
Gadfly: Error During Test at /home/tamas/.julia/dev/Gadfly/test/runtests.jl:77
  Got exception outside of a @test
  MethodError: no method matching findnext(::String, ::Char, ::Int64)
  Closest candidates are:
    findnext(!Matched::Function, ::Any, ::Any) at array.jl:1688
    findnext(::AbstractString, !Matched::AbstractString, ::Integer) at strings/search.jl:256
    findnext(::Any, ::Any) at array.jl:1607
    ...
  Stacktrace:
   [1] resolve(::Measures.BoundingBox{Tuple{Measures.Length{:mm,Float64},Measures.Length{:mm,Float64}},Tuple{Measures.Length{:mm,Float64},Measures.Length{:mm,Float64}}}, ::UnitBox{Float64,Float64,Float64,Float64}, ::Compose.IdentityTransform, ::Compose.JSCallPrimitive) at /home/tamas/.julia/dev/Compose/src/property.jl:437
   [2] (::getfield(Compose, Symbol("##41#42")){Measures.BoundingBox{Tuple{Measures.Length{:mm,Float64},Measures.Length{:mm,Float64}},Tuple{Measures.Length{:mm,Float64},Measures.Length{:mm,Float64}}},UnitBox{Float64,Float64,Float64,Float64},Compose.IdentityTransform})(::Compose.JSCallPrimitive) at ./none:0
   [3] iterate at ./generator.jl:47 [inlined]
   [4] collect at ./array.jl:619 [inlined]
   [5] resolve(::Measures.BoundingBox{Tuple{Measures.Length{:mm,Float64},Measures.Length{:mm,Float64}},Tuple{Measures.Length{:mm,Float64},Measures.Length{:mm,Float64}}}, ::UnitBox{Float64,Float64,Float64,Float64}, ::Compose.IdentityTransform, ::Compose.Property{Compose.JSCallPrimitive}) at /home/tamas/.julia/dev/Compose/src/property.jl:38
   [6] drawpart(::Compose.Image{Compose.PDFBackend}, ::Context, ::Compose.IdentityTransform, ::UnitBox{Float64,Float64,Float64,Float64}, ::Measures.BoundingBox{Tuple{Measures.Length{:mm,Float64},Measures.Length{:mm,Float64}},Tuple{Measures.Length{:mm,Float64},Measures.Length{:mm,Float64}}}) at /home/tamas/.julia/dev/Compose/src/container.jl:408
   [7] drawpart(::Compose.Image{Compose.PDFBackend}, ::Context, ::Compose.IdentityTransform, ::UnitBox{Float64,Float64,Float64,Float64}, ::Measures.BoundingBox{Tuple{Measures.Length{:mm,Float64},Measures.Length{:mm,Float64}},Tuple{Measures.Length{:mm,Float64},Measures.Length{:mm,Float64}}}) at /home/tamas/.julia/dev/Compose/src/container.jl:476
   [8] drawpart(::Compose.Image{Compose.PDFBackend}, ::Compose.Table, ::Compose.IdentityTransform, ::UnitBox{Float64,Float64,Float64,Float64}, ::Measures.BoundingBox{Tuple{Measures.Length{:mm,Float64},Measures.Length{:mm,Float64}},Tuple{Measures.Length{:mm,Float64},Measures.Length{:mm,Float64}}}) at /home/tamas/.julia/dev/Compose/src/container.jl:363
   [9] drawpart(::Compose.Image{Compose.PDFBackend}, ::Context, ::Compose.IdentityTransform, ::UnitBox{Float64,Float64,Float64,Float64}, ::Measures.BoundingBox{Tuple{Measures.Length{:mm,Float64},Measures.Length{:mm,Float64}},Tuple{Measures.Length{:mm,Float64},Measures.Length{:mm,Float64}}}) at /home/tamas/.julia/dev/Compose/src/container.jl:476 (repeats 2 times)
   [10] draw(::Compose.Image{Compose.PDFBackend}, ::Context) at /home/tamas/.julia/dev/Compose/src/container.jl:329
   [11] draw(::Compose.Image{Compose.PDFBackend}, ::Plot) at /home/tamas/.julia/dev/Gadfly/src/Gadfly.jl:851
   [12] macro expansion at ./logging.jl:310 [inlined]
   [13] macro expansion at /home/tamas/.julia/dev/Gadfly/test/runtests.jl:79 [inlined]
   [14] macro expansion at /buildworker/worker/package_linux64/build/usr/share/julia/stdlib/v1.0/Test/src/Test.jl:1083 [inlined]
   [15] top-level scope at /home/tamas/.julia/dev/Gadfly/test/runtests.jl:78
   [16] include at ./boot.jl:317 [inlined]
   [17] include_relative(::Module, ::String) at ./loading.jl:1038
   [18] include(::Module, ::String) at ./sysimg.jl:29
   [19] include(::String) at ./client.jl:388
   [20] top-level scope at none:0
   [21] eval(::Module, ::Any) at ./boot.jl:319
   [22] macro expansion at ./logging.jl:317 [inlined]
   [23] exec_options(::Base.JLOptions) at ./client.jl:219
   [24] _start() at ./client.jl:421
Test Summary: | Pass  Error  Total
Gadfly        |    1      1      2
ERROR: LoadError: Some tests did not pass: 1 passed, 0 failed, 1 errored, 0 broken.

which looks like a bug in Compose

@tlnagy
Copy link
Member

tlnagy commented Sep 7, 2018

So Gadfly's tests caught quite a few bugs in Compose's batching and pango backends neither of which is tested over in that repo. I'm building up another PR to fix them. I've packaged them up here (GiovineItalia/Compose.jl#294) and already merged them.

Also, make sure to be testing this on 1.0 since there are a lot of silent things that slip through on v0.7 that don't give deprecation warnings in my experience.

Gadfly: Error During Test at /home/tamas/.julia/dev/Gadfly/test/testscripts/density_dark.jl:16
  Test threw exception
  Expression: occursin(hex((Gadfly.dark_theme).panel_fill), svg_str_dark)
  UndefVarError: hex not defined
  Stacktrace:
   [1] top-level scope at /home/tamas/.julia/dev/Gadfly/test/testscripts/density_dark.jl:16
   [2] include at ./boot.jl:317 [inlined]
   [3] include_relative(::Module, ::String) at ./loading.jl:1038
   [4] include at ./sysimg.jl:29 [inlined]
   [5] include(::String) at ./loading.jl:1073
   [6] top-level scope at none:0
   [7] eval at ./boot.jl:319 [inlined]
   [8] evalfile(::String, ::Array{String,1}) at ./loading.jl:1069 (repeats 2 times)
   [9] macro expansion at ./logging.jl:308 [inlined]
   [10] macro expansion at /home/tamas/.julia/dev/Gadfly/test/runtests.jl:79 [inlined]
   [11] macro expansion at /buildworker/worker/package_linux64/build/usr/share/julia/stdlib/v1.0/Test/src/Test.jl:1083 [inlined]
   [12] top-level scope at /home/tamas/.julia/dev/Gadfly/test/runtests.jl:78

@tlnagy
Copy link
Member

tlnagy commented Sep 7, 2018

Looks like subplot_grid_free_axis.pdf is crashing too:

[ Info: subplot_grid_free_axis.pdf
ERROR: LoadError: TypeError: non-boolean (Missing) used in boolean context
Stacktrace:
 [1] show(::IOContext{Base.GenericIOBuffer{Array{UInt8,1}}}, ::Gadfly.Aesthetics) at /home/tamas/.julia/dev/Gadfly/src/aesthetics.jl:100
 [2] show_delim_array(::IOContext{Base.GenericIOBuffer{Array{UInt8,1}}}, ::Tuple{Gadfly.Aesthetics}, ::Char, ::Char, ::Char, ::Bool, ::Int64, ::Int64) at ./show.jl:695
 [3] show_delim_array at ./show.jl:680 [inlined]
 [4] show(::IOContext{Base.GenericIOBuffer{Array{UInt8,1}}}, ::Tuple{Gadfly.Aesthetics}) at ./show.jl:714
 [5] show_default(::IOContext{Base.GenericIOBuffer{Array{UInt8,1}}}, ::Any) at ./show.jl:332
 [6] show(::IOContext{Base.GenericIOBuffer{Array{UInt8,1}}}, ::Any) at ./show.jl:315
 [7] #sprint#325(::Pair{Symbol,Bool}, ::Int64, ::Function, ::Function, ::MethodError) at ./strings/io.jl:99
 [8] #sprint at ./none:0 [inlined]
 [9] Test.Error(::Symbol, ::Expr, ::MethodError, ::Array{Union{Ptr{Nothing}, InterpreterIP},1}, ::LineNumberNode) at /buildworker/worker/package_linux64/build/usr/share/julia/stdlib/v1.0/Test/src/Test.jl:144
 [10] macro expansion at /home/tamas/.julia/dev/Gadfly/test/runtests.jl:79 [inlined]
 [11] macro expansion at /buildworker/worker/package_linux64/build/usr/share/julia/stdlib/v1.0/Test/src/Test.jl:1083 [inlined]
 [12] top-level scope at /home/tamas/.julia/dev/Gadfly/test/runtests.jl:78
 [13] include at ./boot.jl:317 [inlined]
 [14] include_relative(::Module, ::String) at ./loading.jl:1038
 [15] include(::Module, ::String) at ./sysimg.jl:29
 [16] include(::String) at ./client.jl:388
 [17] top-level scope at none:0
in expression starting at /home/tamas/.julia/dev/Gadfly/test/runtests.jl:77

signal (11): Segmentation fault

@bjarthur
Copy link
Member Author

bjarthur commented Sep 8, 2018

wow, okay, so far i've only tested on 0.7. will get on 1.0 tomorrow.

re. the Media issue, i'm hoping @pfitzseb can give us a hand. see JunoLab/Juno.jl#125 (comment)

@bjarthur
Copy link
Member Author

bjarthur commented Sep 8, 2018


```
diff --git a/src/geom/label.jl b/src/geom/label.jl
index a26ee82e..a043a7c5 100644
--- a/src/geom/label.jl
+++ b/src/geom/label.jl
@@ -51,9 +51,9 @@ function deferred_label_context(geom::LabelGeometry,
     num_labels = length(aes.label)
 
     if aes.size == nothing
-        padding = fill(theme.point_size, num_labels) .+ theme.label_padding
+        padding = fill(theme.point_size, num_labels) .+ Ref(theme.label_padding)
     else
-        padding = aes.size .+ theme.label_padding
+        padding = aes.size .+ Ref(theme.label_padding)
     end
 
     point_positions = Array{AbsoluteVec2}(undef, 0)
```

i prefer @Mattriks [suggestion](https://github.com/GiovineItalia/Compose.jl/pull/282#issuecomment-419423106) though to modify Measures.jl instead.  i'll submit a PR there unless he beats me to it.

one thing to note is that in a few dozen unit tests the color of some lines or points changed from black to light blue.  can someone please confirm that my julia 0.6 installation isn't screwed up by rendering test/testscripts/point_size_explicit.jl and reporting back the color of the shapes?  light blue is more consistent with the Gadfly color scheme, and that's how it's rendering for me with this PR, but i think in julia 0.6 it was black.

@tlnagy
Copy link
Member

tlnagy commented Sep 8, 2018

i prefer @Mattriks suggestion though to modify Measures.jl instead. i'll submit a PR there unless he beats me to it.

I beat you all both to it :P JuliaGraphics/Measures.jl#18 Make sure to pull the latest master.

can someone please confirm that my julia 0.6 installation isn't screwed up by rendering test/testscripts/point_size_explicit.jl and reporting back the color of the shapes?

I'll look into this

@bjarthur
Copy link
Member Author

removing comprehension broke beeswarm unit test. all other changes made. thanks!

@@ -964,13 +966,13 @@ function apply_statistic_typed(minval::T, maxval::T, vals, size, dsize) where T
minval, maxval
end

function apply_statistic_typed(minval::T, maxval::T, vals::DataArray{T}, size, dsize) where T
function apply_statistic_typed(minval::T, maxval::T, vals::Array{Union{Missing,T}}, size, dsize) where T
Copy link
Contributor

Choose a reason for hiding this comment

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

You should also be able to remove this method and just check !ismissing(val) in the general method above.

src/misc.jl Outdated
mapping = coalesce.(indexin(CategoricalArrays.index(values.pool), levels), 0)
unshift!(mapping, coalesce(findfirst(ismissing, levels), 0))
mapping = Union{Nothing,Int}[coalesce.(indexin(CategoricalArrays.index(values.pool), levels), 0)...]
pushfirst!(mapping, coalesce(findfirst(ismissing, levels), 0))
Copy link
Contributor

Choose a reason for hiding this comment

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

@bjarthur something should be used rather than coalesce as @ararslan noted. The current code does not replace nothing with 0 as it should, so I'm not even sure how it works.

Regarding the use of the comprehension, what bothers me is that you're splatting a potentially very large array, which will force recompiling a specialized function for each length. What error do you get without it?

@nalimilan
Copy link
Contributor

@Mattriks I think that's because CategoricalArray inputs are converted to IndirectArray, which is treated as categorical.

@bjarthur
Copy link
Member Author

the tick differences in the regression tests are due to hard-coding the calls to rand in julia 0.6 as Float32s in 1.0. so nothing to worry about!

@bjarthur
Copy link
Member Author

@nalimilan your latest suggested changes are made. thanks for being persistent.

folks-- unit and regression tests all pass locally. and all of the dependencies have been tagged. i expect this to pass CI and will merge once it does unless i hear otherwise. a post on discourse soliciting field testing will follow.

src/misc.jl Outdated
@@ -404,7 +360,7 @@ discretize_make_ia(values::CategoricalArray) =
discretize_make_ia(values, intersect(push!(levels(values), missing), unique(values)))
discretize_make_ia(values::CategoricalArray, ::Nothing) = discretize_make_ia(values)
function discretize_make_ia(values::CategoricalArray{T}, levels::Vector) where {T}
mapping = Union{Nothing,Int}[coalesce.(indexin(CategoricalArrays.index(values.pool), levels), 0)...]
mapping = something(indexin(CategoricalArrays.index(values.pool), levels), 0)
Copy link
Contributor

@nalimilan nalimilan Sep 12, 2018

Choose a reason for hiding this comment

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

I think this should be something.(...). Also, below coalesce should be replaced with something. It's weird that tests didn't catch it. EDIT: looks like they did, which is good! ;-)

Copy link
Member Author

Choose a reason for hiding this comment

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

i can't reproduce the CI error on a fresh install on linux. nor on my usual os x box. but i've made the suggested changes and pushed. hopefully CI will pass now.

@bjarthur
Copy link
Member Author

i can't reproduce the CI error. could someone else please checkout this branch and give it a try?

@tlnagy
Copy link
Member

tlnagy commented Sep 12, 2018

folks-- unit and regression tests all pass locally. and all of the dependencies have been tagged. i expect this to pass CI and will merge once it does unless i hear otherwise. a post on discourse soliciting field testing will follow.

My one concern is the Media errors, but given that all the tests pass I'm leaning towards merging this into master so people can start using it and finding bugs. We'll need to fix the Media errors prior to tagging though.

i can't reproduce the CI error. could someone else please checkout this branch and give it a try?

I can confirm that unit tests pass locally on Ubuntu 18.04. I'm rebooting the CI and trying it again.

EDIT: I have no idea why the CI tests are failing. It looks like it has to do something with color being discretized into an IndirectArray, but it doesn't seem to fail locally.

plot(x=rand(20), y=rand(20), color=repeat(["A","B"], inner=10),
Guide.colorkey(title="Species", labels=["Name1","Name2"], pos=[0.85w,-0.3h])
)

@tlnagy
Copy link
Member

tlnagy commented Sep 12, 2018

Just noting a weird fact about Gadfly. This works:

a = CategoricalArrays.CategoricalArray([1,2,3,3,3,4,3,2])
plot(x=a, Geom.histogram)

but why should it, when:

isa(a, Gadfly.NumericalOrCategoricalAesthetic) # is false

Hence the typing of aesthetics in aesthetics.jl isn't enforced. I suspect this is because x is currently an untyped keyword argument.

@Mattriks This isn't surprising when you actually look at how Gadfly.NumericalOrCategoricalAesthetic is defined. Now I think we could have a discussion whether this is the proper type union:

julia> Gadfly.NumericalOrCategoricalAesthetic
Union{Nothing, Array{T,1} where T, IndirectArray}

julia> typeof(a) <: IndirectArray
false

julia> b = IndirectArray([1,2,3,3,3,4,3,2]);

julia> isa(b, Gadfly.NumericalOrCategoricalAesthetic)
true

src/misc.jl Outdated
mapping = Union{Nothing,Int}[coalesce.(indexin(CategoricalArrays.index(values.pool), levels), 0)...]
pushfirst!(mapping, coalesce(findfirst(ismissing, levels), 0))
mapping = something.(indexin(CategoricalArrays.index(values.pool), levels), 0)
pushfirst!(mapping, something.(findfirst(ismissing, levels), 0))
Copy link
Contributor

Choose a reason for hiding this comment

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

There shouldn't be a dot here. Most of the time it shouldn't matter, but if elements are also collections themselves this could give weird results.

Copy link
Member

Choose a reason for hiding this comment

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

Do you think this is causing the CI errors?

  MethodError: no method matching IndirectArrays.IndirectArray(::Array{Union{Nothing, Int64},1}, ::Array{String,1})
  Closest candidates are:
    IndirectArrays.IndirectArray(!Matched::AbstractArray{#s12,N} where #s12<:Integer, ::AbstractArray{T,1}) where {T, N} at /home/travis/.julia/packages/IndirectArrays/0Jc04/src/IndirectArrays.jl:26
    IndirectArrays.IndirectArray(::AbstractArray) at /home/travis/.julia/packages/IndirectArrays/0Jc04/src/IndirectArrays.jl:33
  Stacktrace:
   [1] discretize_make_ia(::Array{String,1}, ::Array{String,1}) at /home/travis/build/GiovineItalia/Gadfly.jl/src/misc.jl:351
   [2] discretize(::Array{String,1}, ::Array{String,1}, ::Nothing, ::Bool) at /home/travis/build/GiovineItalia/Gadfly.jl/src/scale.jl:263

Copy link
Contributor

Choose a reason for hiding this comment

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

No, that error is a dispatch bug AFAICT. It's really really weird.

This was referenced Sep 12, 2018
@tlnagy
Copy link
Member

tlnagy commented Sep 12, 2018

So this error should only happen if a value is in values but not in levels because then indexin returns nothing instead of the index. Not sure how that can happen, especially when it passes locally.

@nalimilan
Copy link
Contributor

I confirm the tests pass locally. That Travis failure (both on 1.0 and nightlies) is really scary.

@nalimilan
Copy link
Contributor

So this error should only happen if a value is in values but not in levels because then indexin returns nothing instead of the index. Not sure how that can happen, especially when it passes locally.

No, a different error should be thrown:

julia> Array{UInt8}([1, nothing])
ERROR: MethodError: Cannot `convert` an object of type Nothing to an object of type UInt8

@pfitzseb
Copy link

Re the Media error:

diff --git a/src/Gadfly.jl b/src/Gadfly.jl
index 2cebdda3..7655ac95 100755
--- a/src/Gadfly.jl
+++ b/src/Gadfly.jl
@@ -53,7 +53,7 @@ function __init__()
             show(err)
         end
     else
-        push_theme(Juno.isactive() ? :dark : :default)
+        push_theme(:default)
     end
 end
 
@@ -942,6 +942,14 @@ function show(io::IO, m::MIME"text/html", p::Plot)
     show(io, m, svg)
 end
 
+function show(io::IO, m::MIME"application/juno+plotpane", p::Plot)
+    buf = IOBuffer()
+    svg = SVGJS(buf, Compose.default_graphic_width,
+                Compose.default_graphic_height, false)
+    draw(svg, p)
+    show(io, "text/html", svg)
+end
+
 function show(io::IO, m::MIME"image/svg+xml", p::Plot)
     buf = IOBuffer()
     svg = SVG(buf, Compose.default_graphic_width,
@@ -1090,22 +1098,6 @@ function display(d::REPLDisplay, ::MIME"application/pdf", p::Union{Plot,Compose.
     open_file(filename)
 end
 
-# Display in Juno
-
-import Juno: Juno, @render, media, Media
-
-media(Plot, Media.Plot)
-
-@render Juno.PlotPane p::Plot begin
-    x, y = Juno.plotsize()
-    set_default_plot_size(x*Gadfly.px, y*Gadfly.px)
-    HTML(stringmime("text/html", p))
-end
-
-@render Juno.Editor p::Gadfly.Plot begin
-    Juno.icon("graph")
-end
-
 include("coord.jl")
 include("geometry.jl")
 include("guide.jl")

gets rid of it and allows plots (without interactivity so far) to be displayed in Juno.

src/Gadfly.jl Outdated
@@ -1001,19 +1000,19 @@ This function is handy when rendering by `plot` has been suppressed
with either trailing semi-colon or by calling it within a function.
"""
function display(d::REPLDisplay, p::Union{Plot,Compose.Context})
Copy link

@pfitzseb pfitzseb Sep 12, 2018

Choose a reason for hiding this comment

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

Please don't overload display(::REPLDisplay, ...) for things the REPL can't actually display. Instead define your own GadflyDisplay, define the appropriate display methods for those and pushdisplay it in your __init__.

The reason this is very unfortunate is that e.g. Juno can't display the plot (in a display(::JunoDisplay,...) call) and then fall back to display(::REPLDisplay, x), because that would show the plot again.

@bjarthur
Copy link
Member Author

@pfitzseb the latest commit in this PR applies your patch and adds GadflyDisplay. thanks. let me know if there is anything else i need to do for interactivity in Juno. i really really would like to see that working.

@nalimilan now with one less dot. if this really is a dispatch error, i presume it will be fixed in 1.0.1 and we can merge this PR. if not, is there anything i should change?

@nalimilan
Copy link
Contributor

I think the PR is fine now. But I'm afraid the failure won't be fixed by 1.0.1 since it also happens on nightlies from today. In the interest of turning CI green, it could be worth trying a few potential workarounds. For example, what happens if you change Array{UInt8} to Vector{UInt8} (which is equivalent here), if you replace it with convert(Vector{UInt8}, ...) or if you add ::Vector{UInt8}? You could also call the IndirectArray{UInt8}(...) constructor. If that makes the failure go away, it would be nice to apply it until the bug is fixed, and it would be useful information to file an issue against Julia.

@bjarthur bjarthur merged commit f963564 into GiovineItalia:master Sep 14, 2018
@bjarthur bjarthur deleted the bja/julia07 branch September 14, 2018 11:20
@ararslan
Copy link

🎉 🎉 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Julia 0.7.0-alpha is out
6 participants