[WIP] Density geometry revamp #1157

tlnagy · 2018-05-28T04:37:12Z

Contributor checklist:

I've updated the documentation to reflect these changes
I've added an entry to NEWS.md
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

Implemented changes

This PR introduces a bunch of new features for Geom.density: custom kernels, custom scaling of densities, stacking, trimmed distributions, adjustments of the computed bandwidth, vertical and horizontal orientations
Geom.violin also gets all these features plus the ability to create split violins and ~~group by color~~ (EDIT: this is bigger than this PR) as well
Geom.violin and Geom.density now share the same DensityStatistic. ~~There is still some ugliness here because I infer whether the user wants a density or violin plot based on what variables are initialized in the aesthetic. I'm not wedded to this approach.~~ EDIT: I now rely on a manual setting on orientation
This PR also introduces a general way for grouping by color and by category. I intend to leverage it for Geom.boxplot and maybe even Geom.bar.

Here's a little taste of what's going to be possible soon:

using Distributions, DataFrames, Gadfly
srand(123);
data = vcat(rand(Beta(2,5), 10), 1-rand(Beta(2,5), 10), 0.25+rand(Beta(2,5), 10));
types = repeat([:one, :two], outer=15);
group = vcat(fill(1, 10), fill(0.5, 10), fill(2, 10)); df = DataFrame(data=data, group=group); p = plot(df, x=:group, y=:data, color=types, Geom.violin(split=true, trim=false))

TODO

Rebuild render() for Geom.density
Flesh out edge cases for Geom.violin render()
Add support for conditional density estimates

tlnagy · 2018-06-01T01:08:26Z

We now have the ability to generate conditional density distributions!

using RDatasets, Gadfly
df = dataset("ggplot2", "diamonds")
plot(df, x=:Carat, color=:Cut, Geom.density(scale=:count, position=:fill))

@Mattriks and @bjarthur do you all know how I can adjust the x and y extents from an apply_statistics function? I would like to trim off the excess space in the above plot somehow.

bjarthur · 2018-06-02T11:43:40Z

#781 is related

working on #1152. Note: This is a WIP and currently completely breaks `Geom.density` and `Geom.violin` has several regressions.

this is necessary for allowing user control over ordering in stacked density plots

My original implementation was too clever in that it figured out the orientation of density and violin plots automatically. The logic ended up being quite convoluted and so I switched back to using the standard `orientation` logic and an explicit flag for whether a density plot is a violin or not. This commit also adds the ability to stack either raw densities and to create conditional density distributions.

There are no set of defaults that apply to both density and violin geometries so it's better if they each have their respective defaults

Simple KDEs should now be working

tlnagy · 2018-06-03T22:54:39Z

There's a bug in KernelDensity.jl that will sometimes cause this PR to fail. It would probably be best if my fix (JuliaStats/KernelDensity.jl#52) is merged and tagged prior to merging this PR.

- adds support for horizontal violins - removes manual control over splitting (temp until position code is rewritten)

[ci skip]

bjarthur · 2018-06-04T11:16:11Z

src/statistics.jl

-Estimate the density of `x` at `n` points, and put the result in `x` and `y`.
-Smoothing is controlled by `bandwidth`.  Used by [`Geom.density`](@ref Gadfly.Geom.density).
-"""
-const density = DensityStatistic


to be consistent with the rest of the API, i'd suggest keeping const density = DensityStatistic and moving the docstrings for struct DensityStatistic to the retainedStat.density.

bjarthur · 2018-06-04T11:43:46Z

src/statistics.jl

+    A general statistic for density plots (e.g. KDE plots and violin plots).
+See [`Geom.density`](@ref Gadfly.Geom.density) or [`Geom.violin`](@ref
+Gadfly.Geom.violin) for more details.
+"""


in #1116 i purposely tried to teach new users the GoG way of thinking by making the Geom docstrings explicitly defer explanation of details to the corresponding Stat docstrings for "derived" geometries, not the other way around as you've done here. i feel this is important as it really is a different way of doing things that many find hard to grasp. does that make sense?

also, it would be good to be explicit about which aesthetics this statistic transforms, again so that the flow of data into making a graph is transparent. so in this case x is transformed into x and y and is grouped by color. in #1116 i defined aes2str, to which one can input the output of {input,output}_aesthetics(), to help ensure that docs don't get out of sync with code. might not be useful in this case because of the grouping aesthetic in :color.

Makes sense. I'll correct this.

bjarthur · 2018-06-04T11:47:01Z

docs/src/gallery/geometries.md

+
+```@example
+using Gadfly, RDatasets
+plot(dataset("ggplot2", "diamonds"), x=:Carat, color=:Cut, Geom.density(position=:fill), Guide.title("Conditional density estimate"), Coord.cartesian(ymax=1.0, xmax=5))


this long line will likely create a horizontal slider in the generated doc html. a hard line break would be nice.

Good catch, I had already corrected this locally.

bjarthur · 2018-06-05T11:22:49Z

src/geom/density.jl

+    Geom.violin[(; bandwidth, adjust, kernel, trim, order)]
+
+Draws a violin plot which is a combination of [`Geom.density`](@ref) and
+[`Geom.boxplot`](@ref). This plot type is useful for comparing differences in


i don't see a call to Geom.boxplot in the Geom.violin code. is this docstring correct in this regard?

I meant it stylistically, not technically, but that could change (see #1157 (comment))

bjarthur · 2018-06-05T11:23:45Z

src/geom/density.jl

+nothing # hide
+```
+![](diamonds_violin1.svg)
+"""


no other Geom docstrings currently have examples. i'd suggest moving this to the gallery.

I think this would be nice to change, but it depends on JuliaDocs/Documenter.jl#736 anyway so I'll remove this for now.

bjarthur · 2018-06-05T11:32:21Z

src/geom/density.jl

+"""
+   Geom.density(; bandwidth, adjust, kernel, trim, scale, position, orientation, order)
+
+Draws a kernel density estimate. This is a cousin of [`Geom.histogram`](@ref)


it would be nice to document how Geom.density(Stat.identity) behaved. in other words, what aesthetics does Geom.density directly use.

Good point, I'll add this.

bjarthur · 2018-06-05T11:34:28Z

src/geom/density.jl

+"""
+const density = DensityGeometry
+
+element_aesthetics(::DensityGeometry) = Symbol[]


element_aesthetics should contain :x, :y, and :color, no?

If I don't leave this blank, they are filled with autogenerated values so it's impossible to give useful error messages using Gadfly.assert_aesthetics_defined. I wasn't sure how to get around this so I leave this blank and figure out errors later: https://github.com/GiovineItalia/Gadfly.jl/pull/1157/files#diff-9ec506bf78232ae17d082c22c2e66449R616

bjarthur · 2018-06-05T11:42:26Z

looking forward to having this functionality! in general it looks great.

my only big question is whether separate Geoms are necessary (density and violin) with their own typedefs and render functions, or whether Stat.density could be combined with Geom.line, and Geom.density and Geom.violin could simply be aliases.

sorry for all the hassle resolving the conflicts with #1116.

bjarthur · 2018-06-05T11:46:32Z

also, does this break anything? is it backwards compatible with the former Geom.density and Geom.violin? if not, depwarns are warranted. and we should think about whether to merge it before or after fixing #1130

tlnagy · 2018-06-07T00:59:50Z

Thanks for the review. I won't have time to fix this for a bit and I want to make sure we get it right before merging.

also, does this break anything? is it backwards compatible with the former Geom.density and Geom.violin?

AFAIK, there shouldn't be any regressions or changes to the API. There are however some changes to defaults that I think are better than the current ones. The primary change is that density plots by default now clip to the range of the data (this the same as ggplot2) since it doesn't make sense to predict outside that range.

my only big question is whether separate Geoms are necessary (density and violin) with their own typedefs and render functions

This might be achievable. The primary difficulty is that these two geoms are grouped in very different ways.

Density plots are generally overlaid or stacked on top of each other (varying color), only x and y aesthetics are used.
Violin plots are generally dodged (or split) according to group and color, x, y, and width aesthetics are used.

They are both essentially Compose.polygons, but the offsets and spacing are different enough that gets a little hairy. If anything Geom.boxplot and Geom.violin are probably the most likely to share code.

I think in general we should think about separating out the positioning code into structs because a lot of it is boilerplate. The same positioning code should work for bars, boxplots, violins, etc. This is what ggplot2 does. See http://ggplot2.tidyverse.org/reference/index.html#section-layer-position-adjustment for details

bjarthur · 2018-06-30T12:07:29Z

do you want to merge this before or after we support 0.7? there will likely be merge conflicts if we wait.

tlnagy · 2018-07-01T18:42:48Z

I don't have time right now to revamp this PR so I'm okay with letting it get a bit stale and then finalize it a bit later. There are enough small edge cases that I need to handle that it's better to sit on this till it's ready.

wizofe · 2022-02-06T12:58:07Z

Split violin plot is a very useful function and it seems it's still not part of Gadfly. Any chance of possibility to revamp this PR? Let me know if I could help somehow.

bjarthur · 2022-02-06T13:24:04Z

agreed! @tlnagy ??

tlnagy added 7 commits June 3, 2018 13:54

prep for consolidation of density/violin plot code

4b3c4f2

[WIP] prelim version of the density geometry revamp

944fc29

working on #1152. Note: This is a WIP and currently completely breaks `Geom.density` and `Geom.violin` has several regressions.

preserve insertion order when grouping

ca168c3

this is necessary for allowing user control over ordering in stacked density plots

update doctests for groupby

6b2462e

add a render() to Geom.density

d278888

fix for missed renaming

084bb29

tlnagy force-pushed the tn/density-revamp branch from fe4a3a1 to 084bb29 Compare June 3, 2018 20:58

tlnagy added 2 commits June 3, 2018 15:07

move default values from densitystat to respective geoms

29022de

There are no set of defaults that apply to both density and violin geometries so it's better if they each have their respective defaults

get density estimation to a functional state

0f90c44

Simple KDEs should now be working

tlnagy mentioned this pull request Jun 3, 2018

use improved high numerical precision linspace from Base JuliaStats/KernelDensity.jl#52

Merged

tlnagy added 5 commits June 3, 2018 16:29

fix up violin render()

3e9617c

- adds support for horizontal violins - removes manual control over splitting (temp until position code is rewritten)

fix bandwidth setting example

d3c77ac

flesh out docs and fix doctests

55e4ff8

update docs and examples for new density features

c6b2104

update news

229b9fd

[ci skip]

bjarthur mentioned this pull request Jun 4, 2018

Use DocStringExtensions in Gadfly #1161

Closed

bjarthur reviewed Jun 4, 2018

View reviewed changes

bjarthur reviewed Jun 5, 2018

View reviewed changes

bjarthur mentioned this pull request Jun 12, 2018

histogram(density = true) and x_discrete(labels = ...) incompatible ? #1163

Closed

kilianbreathnach mentioned this pull request Sep 11, 2018

Julia 0.7.0-alpha is out #1158

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] Density geometry revamp #1157

[WIP] Density geometry revamp #1157

tlnagy commented May 28, 2018 •

edited

Loading

tlnagy commented Jun 1, 2018

bjarthur commented Jun 2, 2018

tlnagy commented Jun 3, 2018

bjarthur Jun 4, 2018

tlnagy Jun 7, 2018

bjarthur Jun 4, 2018

tlnagy Jun 7, 2018

bjarthur Jun 4, 2018

tlnagy Jun 7, 2018

bjarthur Jun 5, 2018

tlnagy Jun 7, 2018

bjarthur Jun 5, 2018

tlnagy Jun 7, 2018

bjarthur Jun 5, 2018

tlnagy Jun 7, 2018

bjarthur Jun 5, 2018

tlnagy Jun 7, 2018

bjarthur commented Jun 5, 2018

bjarthur commented Jun 5, 2018

tlnagy commented Jun 7, 2018

bjarthur commented Jun 30, 2018

tlnagy commented Jul 1, 2018

wizofe commented Feb 6, 2022

bjarthur commented Feb 6, 2022

[WIP] Density geometry revamp #1157

Are you sure you want to change the base?

[WIP] Density geometry revamp #1157

Conversation

tlnagy commented May 28, 2018 • edited Loading

Contributor checklist:

Implemented changes

TODO

tlnagy commented Jun 1, 2018

bjarthur commented Jun 2, 2018

tlnagy commented Jun 3, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bjarthur commented Jun 5, 2018

bjarthur commented Jun 5, 2018

tlnagy commented Jun 7, 2018

bjarthur commented Jun 30, 2018

tlnagy commented Jul 1, 2018

wizofe commented Feb 6, 2022

bjarthur commented Feb 6, 2022

tlnagy commented May 28, 2018 •

edited

Loading