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

WIP: update for julia 0.6 #973

Merged
merged 1 commit into from
Apr 13, 2017
Merged

Conversation

bjarthur
Copy link
Member

@bjarthur bjarthur commented Apr 7, 2017

tests are still failing to a macro call in Compose:

while loading /Users/arthurb/.julia/v0.6/Gadfly/test/runtests.jl, in expression starting on line 202
FAILED!
ERROR: LoadError: UndefVarError: codes not defined
Stacktrace:
 [1] macro expansion at /Users/arthurb/.julia/v0.6/Compose/src/misc.jl:125 [inlined]
 [2] jscall(::Array{String,1}, ::Array{Array{Measures.Measure,1},1}) at /Users/arthurb/.julia/v0.6/Compose/src/property.jl:587 (repeats 2 times)
 [3] (::Gadfly.Guide.##3#11{Array{Int64,1},Array{ColorTypes.Color,1},DataStructures.OrderedDict{ColorTypes.Color,AbstractString},Gadfly.Scale.#labeler#66,Compose.Context,Gadfly.Theme,Measures.Length{:mm,Float64},Measures.Length{:mm,Float64}})(::Compose.ParentDrawContext) at /Users/arthurb/.julia/v0.6/Gadfly/src/guide.jl:277

can anyone give me a hint here?

there is still also the show / display code to port.

@tlnagy
Copy link
Member

tlnagy commented Apr 8, 2017

That's not a very helpful error message...

@bjarthur bjarthur force-pushed the bja/julia06 branch 2 times, most recently from 0484ff5 to 53c7380 Compare April 9, 2017 13:06
@tlnagy
Copy link
Member

tlnagy commented Apr 9, 2017

Somehow I think that codes is not passing properly to this function and then the inner macro call. No idea why that is happening now. https://github.com/GiovineItalia/Compose.jl/blob/7ea1616cc084e6426bb5a6db422d564328cd6edd/src/property.jl#L585-L591

@bjarthur bjarthur force-pushed the bja/julia06 branch 2 times, most recently from 4b42487 to 112ff0e Compare April 10, 2017 11:39
@codecov-io
Copy link

codecov-io commented Apr 10, 2017

Codecov Report

Merging #973 into master will increase coverage by 0.04%.
The diff coverage is 78.62%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #973      +/-   ##
==========================================
+ Coverage   78.65%   78.69%   +0.04%     
==========================================
  Files          33       33              
  Lines        4000     4008       +8     
==========================================
+ Hits         3146     3154       +8     
  Misses        854      854
Impacted Files Coverage Δ
src/geometry.jl 71.42% <ø> (ø) ⬆️
src/coord.jl 83.65% <ø> (ø) ⬆️
src/mapping.jl 20.15% <0%> (-0.16%) ⬇️
src/format.jl 0% <0%> (ø) ⬆️
src/geom/rectbin.jl 70.58% <0%> (ø) ⬆️
src/geom/point.jl 97.56% <100%> (ø) ⬆️
src/geom/ribbon.jl 100% <100%> (ø) ⬆️
src/aesthetics.jl 76.69% <100%> (ø) ⬆️
src/scale.jl 85.43% <100%> (+0.39%) ⬆️
src/geom/bar.jl 91.66% <100%> (ø) ⬆️
... and 17 more

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 5a129fb...994ffa1. Read the comment docs.

@bjarthur
Copy link
Member Author

bjarthur commented Apr 10, 2017

all tests pass locally now.

there are still some deprecation warnings though, mostly from FixedSizeArrays.jl, which Contour.jl uses. we should consider switching to StaticArrays.jl at some point. difficulty there is that it has dropped support for 0.5.

Media.jl also throws a couple warnings.

i would still suggest not merging this PR until folks have actually used it. i have not visually checked the appearance of any plots yet to make sure they look okay.

@bjarthur
Copy link
Member Author

bjarthur commented Apr 10, 2017

wait, nevermind, the codes not defined error has reared it's head again...

@bjarthur
Copy link
Member Author

the codes not defined error was a macro hygiene problem. not sure how it ever worked on julia 0.5. adding a few esc() got it working on 0.6.

now the tests are stuck on a world age problem...

@tlnagy
Copy link
Member

tlnagy commented Apr 11, 2017

Awesome! Thanks for finding that out. There seem to be a couple bugs in 0.5 that Gadfly/Compose relied on that were fixed in 0.6

@bjarthur
Copy link
Member Author

bjarthur commented Apr 12, 2017

the world age problem is now fixed, and all tests pass locally on julia 0.5 and 0.6!

would be great if someone could give it a try. you'll need to checkout master on Loess, Hexagons, and Showoff, and checkout bja/julia06 on Compose and Gadfly.

there are still a couple depwarns from the Media.jl dependency, which is for Juno.

and a dozen or so depwarns remain from FixedSizeArrays.jl, which Contour.jl uses. the plan is not to ever upgrade FixedSizeArrays.jl to julia 0.6, but rather to migrate everything that depends on it to StaticArrays.jl.

there are also quite a few broadcast depwarns, and a handful from cycle. the file/line# info is not specific enough for me fix them. i'm guessing they come from some dependency, perhaps FixedSizeArrays.

at any rate, as soon as my Compose PR is merged and tagged, the travis tests should pass for this PR on nightly, and i think we can merge.

@tlnagy
Copy link
Member

tlnagy commented Apr 12, 2017

Is StaticArrays a drop in replacement for FixedSizeArrays? Because then we could move over to StaticArrays for both 0.5 and 0.6. Otherwise, we might have play around with conditional dependencies or something.

@bjarthur
Copy link
Member Author

bjarthur commented Apr 12, 2017

the names of types have changed, but otherwise it's drop-in. whenever someone gets around to migrating Contour from FixedSizeArrays to StaticArrays, Gadfly will either have to drop support for julia 0.5 or add an upper bound on the Contour version. until then, we just have to live with a few deprecation warnings.

@tlnagy
Copy link
Member

tlnagy commented Apr 12, 2017

StaticArrays appears to have 0.5 as the lower bound so we could probably migrate Contour right now without dropping 0.5 support.

@bjarthur
Copy link
Member Author

this says 0.6-. also, Contour.jl is in the process of being moved to JuliaGeometry.

@tlnagy
Copy link
Member

tlnagy commented Apr 12, 2017

Ah, I missed that. I just read the README, but I guess that was out of date. I guess the moment that 0.6 drops we can make a final release for 0.5 and then move master solely to 0.6 and switch to staticarrays.

@bjarthur bjarthur closed this Apr 12, 2017
@bjarthur bjarthur reopened this Apr 12, 2017
@bjarthur bjarthur merged commit ac57691 into GiovineItalia:master Apr 13, 2017
@ararslan
Copy link

Does anything else need to happen before you guys can make a 0.6-compatible tag of Gadfly?

@bjarthur
Copy link
Member Author

someone should check that it works in the various IDEs... Juno, IJulia, JuliaBox. i've only tested on the REPL. specifically, Media.jl throws a few dep warns.

@bjarthur bjarthur deleted the bja/julia06 branch April 15, 2017 11:41
@ararslan
Copy link

IIRC the version of 0.6 on JuliaBox is pretty old so whether Gadfly works there is probably not entirely representative. If I may, I'd recommend that if it works in the REPL you'd might as well tag a version since mostly works is better than not working at all, and as Tony says, "tags are cheap."

@bjarthur
Copy link
Member Author

@tlnagy @Mattriks either of you have any plans to submit PRs in the imminent future? if not, we should tag a release.

@Mattriks
Copy link
Member

Yes I hope to submit Geom.vectorfield (#608 & #791) say next week sometime.

@tlnagy
Copy link
Member

tlnagy commented Apr 20, 2017

As @tkelman says, tagging is cheap. Lets go ahead and go forward with tagging a release so we have 0.6 compat.

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.

5 participants