Enable plotting of Unitful values #1105

non-Jedi · 2018-03-16T15:06:48Z

Following test case works:

julia> using Gadfly; using Unitful;

julia> p = plot(x=[1;2;3;4]u"s", y=[2;4;8;16], Geom.line);

julia> draw(PNG("gadfly_unitful_test.png"), p);

Generates:

Played around with it a bit afterward, and I didn't run into any other breaking issues. Would be nice if the ylabel showed units like the xlabel, but this at least makes Gadfly compatible with Unitful (and generally with well-behaved custom number types).

Closes #1104

non-Jedi · 2018-03-16T15:18:24Z

@bjarthur do you need me to include the fixes to the y-axis label with this PR?
This is a relatively simple bugfix, but I wouldn't know really where to start on
that one. I plan on fixing that piece eventually, but it may be awhile...

bjarthur · 2018-03-16T17:11:09Z

by fixing the y-axis label, to you actually mean y-axis ticks? if so, the relevant code is here. compare with the x-axis tick code immediately above to see what the difference might be.

also, if we're serious about supporting this in future, you should add your simple test case above to test/testscripts/. this would necessitate adding Unitful to test/REQUIRE.

@tlnagy what do you think of adding a test dependency like this? previously you had mentioned refactoring Measures out in favor of Unitful i believe.

tlnagy · 2018-03-16T20:27:06Z

what do you think of adding a test dependency like this? previously you had mentioned refactoring Measures out in favor of Unitful i believe.

I don't have any problems with adding a test dependency since that wouldn't bloat our dependency graph for a normal install.

We should definitely add a test for using Unitful.jl with Gadfly in this PR to make sure that we don't break compatibility in the future.

Would be nice if the ylabel showed units like the xlabel

You're not testing unitful quantities on the y axis though? y=[2;4;8;16] are Ints not Unitful quantities?

@non-Jedi Some other edge cases: does this work with unitful quantities on the y axis? On both axes?

tlnagy · 2018-03-16T20:28:35Z

do you need me to include the fixes to the y-axis label with this PR?

We should make sure that this works in all basic cases before merging this PR. We can leave this open for one of us (or someone else) to update if you don't have time. You can keep using your local branch with this modification for your immediate plotting needs.

tlnagy · 2018-03-16T21:03:18Z

Checking out this PR seems to work for the y axis too?

p = plot(x=[1;2;3;4]u"s", y=[2;4;8;16]u"kg", Geom.line)

tlnagy · 2018-03-16T21:11:18Z

One thing I would like to point out is that it is custom to have the units in axes labels instead in the ticks. Like so

p = plot(x=[1;2;3;4], y=[2;4;8;16]u"kg", Geom.line, Guide.ylabel("y (kg)"))

but here the information is duplicated and there is no easy way to hide the units in the ticks

p = plot(x=[1;2;3;4], y=[2;4;8;16]u"kg", Geom.line, Guide.yticks(ticks=[2,4,6,8]))

DimensionError: 2 and 2 kg are not dimensionally compatible.

Stacktrace:
 [1] min(::Unitful.Quantity{Int64,Unitful.Dimensions{(Unitful.Dimension{:Mass}(1//1),)},Unitful.FreeUnits{(Unitful.Unit{:Gram,Unitful.Dimensions{(Unitful.Dimension{:Mass}(1//1),)}}(3, 1//1),),Unitful.Dimensions{(Unitful.Dimension{:Mass}(1//1),)}}}, ::Int64) at ./operators.jl:361
 [2] apply_statistic(::Gadfly.Stat.TickStatistic, ::Dict{Symbol,Gadfly.ScaleElement}, ::Gadfly.Coord.Cartesian, ::Gadfly.Aesthetics) at /Users/tamasnagy/.julia/v0.6/Gadfly/src/statistics.jl:784
 [3] apply_statistics(::Array{Gadfly.StatisticElement,1}, ::Dict{Symbol,Gadfly.ScaleElement}, ::Gadfly.Coord.Cartesian, ::Gadfly.Aesthetics) at /Users/tamasnagy/.julia/v0.6/Gadfly/src/statistics.jl:41
 [4] render_prepare(::Gadfly.Plot) at /Users/tamasnagy/.julia/v0.6/Gadfly/src/Gadfly.jl:708
 [5] render(::Gadfly.Plot) at /Users/tamasnagy/.julia/v0.6/Gadfly/src/Gadfly.jl:761
 [6] show at /Users/tamasnagy/.julia/v0.6/Gadfly/src/Gadfly.jl:965 [inlined]
 [7] limitstringmime(::MIME{Symbol("image/svg+xml")}, ::Gadfly.Plot) at /Users/tamasnagy/.julia/v0.6/IJulia/src/inline.jl:24
 [8] display_dict(::Gadfly.Plot) at /Users/tamasnagy/.julia/v0.6/IJulia/src/execute_request.jl:33
 [9] (::Compat.#inner#17{Array{Any,1},IJulia.#display_dict,Tuple{Gadfly.Plot}})() at /Users/tamasnagy/.julia/v0.6/Compat/src/Compat.jl:488
 [10] execute_request(::ZMQ.Socket, ::IJulia.Msg) at /Users/tamasnagy/.julia/v0.6/IJulia/src/execute_request.jl:186
 [11] (::Compat.#inner#17{Array{Any,1},IJulia.#execute_request,Tuple{ZMQ.Socket,IJulia.Msg}})() at /Users/tamasnagy/.julia/v0.6/Compat/src/Compat.jl:488
 [12] eventloop(::ZMQ.Socket) at /Users/tamasnagy/.julia/v0.6/IJulia/src/eventloop.jl:8
 [13] (::IJulia.##14#17)() at ./task.jl:335

non-Jedi · 2018-03-16T21:51:02Z

You're not testing unitful quantities on the y axis though? y=[2;4;8;16] are
Ints not Unitful quantities?

🤦 yep, that explains it.

I'll plan on pushing a commit to this branch sometime this weekend testing a
bunch of potential codepaths for Unitful use. No promises after that.

To be honest, I see the current content of this PR as a bugfix with the topic of
things like turning off display of units as ticks as enhancements.

non-Jedi · 2018-03-17T00:28:03Z

Interesting note: using Unitful here enforces the no-multiple axes principle
articulated in #373 much more
strictly. It also may provide a clean api for secondary axes if anyone wants to
implement that. Since it locks the scale of the axis to the type of the
displayed data, you could have secondary axes generated based on mismatches in
type of y-values in different layers without explicitly specifying a secondary axis at all.

codecov-io · 2018-03-17T01:52:43Z

Codecov Report

Merging #1105 into master will increase coverage by 9.45%.
The diff coverage is 90.9%.

@@            Coverage Diff             @@
##           master    #1105      +/-   ##
==========================================
+ Coverage   74.07%   83.53%   +9.45%     
==========================================
  Files          35       35              
  Lines        4042     4032      -10     
==========================================
+ Hits         2994     3368     +374     
+ Misses       1048      664     -384

Impacted Files	Coverage Δ
src/ticks.jl	`80.57% <100%> (+12.23%)`	⬆️
src/scale.jl	`86.89% <100%> (+4.69%)`	⬆️
src/geom/hvabline.jl	`93.84% <80%> (+4.14%)`	⬆️
src/misc.jl	`50% <0%> (+1.92%)`	⬆️
src/geom/beeswarm.jl	`78.75% <0%> (+3.75%)`	⬆️
src/aesthetics.jl	`78.81% <0%> (+4.23%)`	⬆️
src/bincount.jl	`83.5% <0%> (+5.15%)`	⬆️
src/geom/errorbar.jl	`97.22% <0%> (+5.55%)`	⬆️
... and 23 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 7c6e899...e5436d0. Read the comment docs.

non-Jedi · 2018-03-19T01:12:38Z

@bjarthur @tlnagy FYI, from the perspective of getting all basic Unitful
plotting functionality working, I believe this PR is complete now. Still some
enhancements to be made wrt things like putting units in axis labels instead of
tick labels as discussed above, but from my perspective that could be done in a
later PR (which I'm planning on doing... eventually...). Your call, of course,
whether to merge now or wait for those enhancements.

src/scale.jl

src/geom/hvabline.jl

bjarthur · 2018-03-25T14:49:33Z

the changes here are small, but the implications are much bigger i think. i need more time to understand the impact of having non-Float64 aesthetics. @tlnagy @Mattriks thoughts?

non-Jedi · 2018-03-31T17:35:40Z

FWIW, it was @dcjones intention for non-Float64 to work back in 2013: #77

bjarthur · 2018-04-01T17:44:57Z

ahah, so nominally unitful should've worked all along, and the changes you had to make in this PR were simply to fix oversights due to lack of unit testing combining Percent with hvabline, scales, ticks, etc.

okay, so i'm fine then with merging.

my only reservation is adding Unitful to test/REQUIRE, because 1. there are other packages which also provide unit types; why should we single out that one?, and 2. Gadfly already has a huge dependency tree which many complain about.

would you consider adding a gridstack to test/percent.jl and moving your three test scripts over there? you also need to resolve conflicts.

thanks for this! looks great.

tlnagy · 2018-04-03T23:58:28Z

my only reservation is adding Unitful to test/REQUIRE, because 1. there are other packages which also provide unit types; why should we single out that one?

I think Unitful is the defactor standard at this point. I'm okay with adding it.

Gadfly already has a huge dependency tree which many complain about.

This is only in test/REQUIRE not in REQUIRE. This would only bloat things if you want to run tests locally. Assuming we're doing our job properly, users shouldn't need to do that so I'm okay with adding it.

Main things remaining before this is ready:

Doc examples
Add to NEWS.md

bjarthur · 2018-04-04T00:22:55Z

I think Unitful is the defactor standard at this point

it is now, but for how long? there have been many others in the past. i'd rather gadfly be agnostic.

tlnagy · 2018-04-04T00:42:44Z

it is now, but for how long? there have been many others in the past. i'd rather gadfly be agnostic.

If a new one emerges, we can always switch the tests over to the new library. None of the changes this PR makes in src/ are specific to Unitful and we can always switch libraries in test/ without breaking anything.

bjarthur · 2018-04-04T11:41:44Z

do bug fixes like this warrant a mention in NEWS? i'd suggest holding off on that until the units appear in the label and not the ticks. that would be a big enough step that a whole new section in the Tutorial dedicated to units could be added too.

also, i am still really in favor of just having the unit tests in this PR use the Percent type that dcjones already has in the tests. the code in the tests as they are now could be saved and used for the docs later.

tlnagy · 2018-04-04T17:26:27Z

do bug fixes like this warrant a mention in NEWS?

I was thinking of it more like we're adding feature (e.g. Gadfly now supports unitful values too!)

also, i am still really in favor of just having the unit tests in this PR use the Percent type that dcjones already has in the tests. the code in the tests as they are now could be saved and used for the docs later.

If you feel strongly about this then we shouldn't include Unitful in the tests. You're talking about this, right?

Gadfly.jl/test/testscripts/percent.jl

Lines 7 to 9 in 602f2d6

    
           struct Percent 
        
               value::Float64 
        
           end

bjarthur · 2018-04-04T18:02:40Z

as @non-Jedi pointed out, units have nominally worked all the way back to #77. they just broke over time because of lack of testing. so yes, let's just expand testscripts/percent.jl to test everything that this PR fixes. then we don't need Unitful in test/REQUIRE and Gadfly code and tests are agnostic. i don't mind using Unitful in the docs, but think it looks funny to have the units in the ticks not labels, so would wait to do so until that is changed.

non-Jedi · 2018-04-06T01:18:46Z

On the subject of units on ticks vs labels, it looks like nicely formatting this
kind of thing is exactly what https://github.com/dcjones/Showoff.jl is for,
which appears to still be a dependency of Gadfly. I haven't messed with it
myself, but getting Unitful values to show on ticks with just the bare numbers
should just be a matter of defining
Showoff.showoff(::AbstractArray{Unitful.Quantity}). So even that functionality
might just be a matter of documenting stuff better, though I haven't messed with
Showoff myself to check.

That said, if there's consensus on not testing against Unitful, I'll go ahead
and migrate all the new tests into the percent testscript as best I can. And
then rebase on master of course. Will do as soon as I get a chance.

non-Jedi · 2018-04-06T01:22:34Z

(On another note, since Showoff is still a dependency of Gadfly and not really used by anything else afaict, shouldn't it get moved into the GiovineItalia organization?)

tlnagy · 2018-04-06T01:43:51Z

What's wrong with where it is?

julia> Pkg.dependents("Showoff")
4-element Array{AbstractString,1}:
 "Plots"
 "Gadfly"
 "Mamba"
 "MixedModels"

If you want to make a PR against Showoff adding that convenience function, I would be happy to review it and merge.

Thanks for your efforts here!

non-Jedi · 2018-04-06T02:06:03Z

No way of doing it without making Showoff depend on Unitful which would then end up pulling in Unitful to Gadfly where we're trying to avoid it. :/ Unless y'all know how to do some sort of an optional dependency, I was thinking that I would just write some more thorough documentation about overriding Showoff.showoff.

(Thanks for showing me Pkg.dependendents. That's a useful function I had somehow missed until now.)

bjarthur · 2018-04-06T11:06:05Z

optional dependencies will soon be added to Pkg3. i look forward to making Gadfly optionally depend on Cairo and DataFrames.

in this case though, an example in the documentation of how to use Showoff with Unitful would be great. i'd even devote an entirely new section in the Tutorial or Manual to it. could do that now and not wait until we've figured out how to automatically add it to labels.

tlnagy · 2018-08-03T01:17:29Z

Hey @non-Jedi, we could use Requires.jl in Showoff.jl to load all the Unitful.jl specific code. This way we'll only depend on Unitful.jl in testing Showoff.jl

This is also generally more correct. convert(T, one(T)) isn't necessarily defined for any arbitrary T, but oneunit(T) should be.

non-Jedi · 2018-12-05T20:01:30Z

For what it's worth, this is now good to go with Julia 1.0. Only thing blocking merge now is the request to remove the test dependency on Unitful. I hope to have time to clean that up sometime in the next month or so, but no promises.

non-Jedi · 2018-12-15T23:00:37Z

I'm not going to be able to test this effectively without Unitful. As an example, take a look at https://github.com/GiovineItalia/Gadfly.jl/pull/1105/files#diff-d568a2b9faaba434884de694f9537f19R15 where I'm plotting distance versus time and giving slope in terms of velocity; that's too much of Unitful to reimplement just for running a test. @bjarthur, could you please reconsider allowing Unitful as a test dependency?

bjarthur · 2018-12-16T13:09:18Z

thanks for this! would be great to some day document how to use Showoff to get the units in the labels and out of the ticks.

tlnagy changed the title ~~Make optimize_ticks_typed compatible with Gadfly~~ Make optimize_ticks_typed compatible with Unitful Mar 16, 2018

non-Jedi force-pushed the fix_unitful branch 3 times, most recently from 86a85e7 to a40eb33 Compare March 18, 2018 19:09

bjarthur reviewed Mar 19, 2018

View reviewed changes

src/scale.jl Show resolved Hide resolved

bjarthur reviewed Mar 19, 2018

View reviewed changes

src/geom/hvabline.jl Show resolved Hide resolved

non-Jedi changed the title ~~Make optimize_ticks_typed compatible with Unitful~~ Enable plotting of Unitful values Mar 19, 2018

non-Jedi force-pushed the fix_unitful branch from 4291e81 to 9b005c5 Compare April 7, 2018 01:50

bjarthur mentioned this pull request Apr 27, 2018

tagging 0.7 #1140

Closed

non-Jedi force-pushed the fix_unitful branch from 9b005c5 to 5804f9b Compare June 11, 2018 19:30

non-Jedi added 5 commits December 5, 2018 12:48

Make optimize_ticks_typed compatible with Gadfly

826973d

This is also generally more correct. convert(T, one(T)) isn't necessarily defined for any arbitrary T, but oneunit(T) should be.

Fix failing test by adding necessary convert method

688ed74

Add a bunch of failing tests for Unitful plotting

c705668

Attempt to fix broken unitful_color.jl tests

82d381e

Fix failing unitful_geom.jl tests

36ab6ba

non-Jedi force-pushed the fix_unitful branch from 5804f9b to 36ab6ba Compare December 5, 2018 17:51

Fixup Unitful patch for Julia 1.0 compatibility

e5436d0

non-Jedi force-pushed the fix_unitful branch from fbd7a0c to e5436d0 Compare December 10, 2018 20:08

bjarthur merged commit 2dfff47 into GiovineItalia:master Dec 16, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable plotting of Unitful values #1105

Enable plotting of Unitful values #1105

non-Jedi commented Mar 16, 2018 •

edited

Loading

non-Jedi commented Mar 16, 2018

bjarthur commented Mar 16, 2018

tlnagy commented Mar 16, 2018

tlnagy commented Mar 16, 2018

tlnagy commented Mar 16, 2018

tlnagy commented Mar 16, 2018

non-Jedi commented Mar 16, 2018

non-Jedi commented Mar 17, 2018 •

edited

Loading

codecov-io commented Mar 17, 2018 •

edited

Loading

non-Jedi commented Mar 19, 2018

bjarthur commented Mar 25, 2018

non-Jedi commented Mar 31, 2018

bjarthur commented Apr 1, 2018

tlnagy commented Apr 3, 2018

bjarthur commented Apr 4, 2018

tlnagy commented Apr 4, 2018

bjarthur commented Apr 4, 2018 •

edited

Loading

tlnagy commented Apr 4, 2018

bjarthur commented Apr 4, 2018

non-Jedi commented Apr 6, 2018

non-Jedi commented Apr 6, 2018

tlnagy commented Apr 6, 2018

non-Jedi commented Apr 6, 2018 •

edited

Loading

bjarthur commented Apr 6, 2018

tlnagy commented Aug 3, 2018

non-Jedi commented Dec 5, 2018

non-Jedi commented Dec 15, 2018

bjarthur commented Dec 16, 2018

Enable plotting of Unitful values #1105

Enable plotting of Unitful values #1105

Conversation

non-Jedi commented Mar 16, 2018 • edited Loading

non-Jedi commented Mar 16, 2018

bjarthur commented Mar 16, 2018

tlnagy commented Mar 16, 2018

tlnagy commented Mar 16, 2018

tlnagy commented Mar 16, 2018

tlnagy commented Mar 16, 2018

non-Jedi commented Mar 16, 2018

non-Jedi commented Mar 17, 2018 • edited Loading

codecov-io commented Mar 17, 2018 • edited Loading

Codecov Report

non-Jedi commented Mar 19, 2018

bjarthur commented Mar 25, 2018

non-Jedi commented Mar 31, 2018

bjarthur commented Apr 1, 2018

tlnagy commented Apr 3, 2018

bjarthur commented Apr 4, 2018

tlnagy commented Apr 4, 2018

bjarthur commented Apr 4, 2018 • edited Loading

tlnagy commented Apr 4, 2018

bjarthur commented Apr 4, 2018

non-Jedi commented Apr 6, 2018

non-Jedi commented Apr 6, 2018

tlnagy commented Apr 6, 2018

non-Jedi commented Apr 6, 2018 • edited Loading

bjarthur commented Apr 6, 2018

tlnagy commented Aug 3, 2018

non-Jedi commented Dec 5, 2018

non-Jedi commented Dec 15, 2018

bjarthur commented Dec 16, 2018

non-Jedi commented Mar 16, 2018 •

edited

Loading

non-Jedi commented Mar 17, 2018 •

edited

Loading

codecov-io commented Mar 17, 2018 •

edited

Loading

bjarthur commented Apr 4, 2018 •

edited

Loading

non-Jedi commented Apr 6, 2018 •

edited

Loading