8000
Skip to content

Removing direction constraint from geoms - #3506

Merged
thomasp85 merged 52 commits into
tidyverse:masterfrom
thomasp85:no-direction
Oct 1, 2019
Merged

Removing direction constraint from geoms#3506
thomasp85 merged 52 commits into
tidyverse:masterfrom
thomasp85:no-direction

Conversation

@thomasp85
@thomasp85 thomasp85 commented Aug 28, 2019
Copy link
Copy Markdown
Member

Some geoms have directionality constraint, e.g. geom_histogram() that requires binning along the x-axis. The gospel has been to use coord_flip() if one wanted binning along the y-axis, but that comes with a set of limitations. Further the constraint seems arbitrary from a user perspective.

This PR explores the possibility of making relevant geoms work in either direction. Hopefully this can completely remove the need for ggstance.

Geoms, stats, and positions under consideration

  • geom_bar
  • geom_histogram
  • geom_violin
  • geom_col
  • geom_linerange()
  • geom_pointrange()
  • geom_errorbar()
  • geom_crossbar()
  • geom_boxplot()
  • geom_density()
  • geom_area()
  • geom_ribbon()
  • geom_line()
  • geom_smooth()
  • stat_bin()
  • stat_boxplot()
  • stat_count()
  • stat_density()
  • stat_ydensity() (consider renaming this for clarity)
  • stat_summary()
  • position_dodge()
  • position_dodge2()
  • position_fill()
  • position_stack()
  • position_jitterdodge()

Please add to this list if anything comes up. @clauswilke when we have settled on this it would be great if ggridges could be updated to reflect this as well

@thomasp85
Copy link
Copy Markdown
Member Author

One thing to consider is how to deal with ambiguity in directional geoms. geom_col() for instance could be used in situations where the data can be interpreted in either direction. Should we add a direction argument that default to NA (i.e. we try to guess) but can be used to overwrite guessing

@thomasp85
Copy link
Copy Markdown
Member Author

This should also deprecate geom_errorbarh

@clauswilke
Copy link
Copy Markdown
Member

Could you describe your overarching strategy/expected API? In an ideal world, the stat (e.g. density) shouldn't have to know about x and y at all and just produce two output columns, one for the dependent and one for the independent variable. These could then be mapped to x and y however one wants. However, I haven't been able to make this possible in the existing ggplot2 stat and geom framework. So I assume you're doing something else.

Also, are there any parts of the logic that could be encapsulated into reusable functions or maybe into a ggproto superclass, so that not that much code duplication is required?

@thomasp85
Copy link
Copy Markdown
Member Author

This is very much WIP at the moment as I’m exploring what is possible. Don’t expect the current state of the code to prevail.

The overarching strategy is to let the stats (and geoms) sniff out the wanted orientation from the mapping of the aesthetics. Many are quite easy such as histograms where you just look at whether x or y is mapped. Others need inspection of the data types mapped to x and y to figure out which one is discrete. Others still are fully ambiguous and we’ll need to figure out what to do there.

The plan and hope is that it should “just work” by mapping the variables to the right aesthetics, but whether this is possible is open

@clauswilke
Copy link
Copy Markdown
Member

Auto-detection + optional manual override should work fine.

@thomasp85
Copy link
Copy Markdown
Member Author

yeah, that was my plan as well... don't know if we want manual override for all or just the ones where ambiguity exist though

@thomasp85
thomasp85 commented Aug 29, 2019
Copy link
Copy Markdown
Member Author

@clauswilke can you remember why you created the GeomCol class? It seems identical to the GeomBar class it subclasses...

AFAIK, geom_col() should just be StatIdentity and GeomBar, right?

@thomasp85
Copy link
Copy Markdown
Member Author

One annoying side effect of this is that many of these stats/geoms have to have their required_aes field removed as they can take two non-overlapping sets of aesthetics to work. Would be nice if this could somehow be specified so we don't loose the self-documenting feature of the field (along with the error handling)

@thomasp85
Copy link
Copy Markdown
Member Author

One way to address the problem of removing required aesthetics will be to add another field to geoms and stats (required_aes_alt, or something) that will be checked and handled along with the primary field... What say you @hadley ?

@clauswilke
Copy link
Copy Markdown
Member

I didn't create GeomCol, I'm just the last one who touched it. I looked at the code right now and compared it to GeomBar and I don't see any differences. Maybe they were different at some point and converged.

@thomasp85
Copy link
Copy Markdown
Member Author

Oh yeah. It was @hrbrmstr. Got it mixed up

@hadley
hadley commented Aug 29, 2019
Copy link
Copy Markdown
Member

Maybe you could make all these Geoms inherit from (say) GeomFlippable and then it could provide custom required aesthetic logic?

I don't see an obvious way to update the documentation generation, ideally we'd special case something where it would generate "x or y".

Or maybe a better option would be to introduce some special sentinel value required_aes = c("x|y")?

@mjskay
mjskay commented Aug 29, 2019
Copy link
Copy Markdown
Contributor

Can I offer some unsolicited thoughts after seeing this on Twitter? :) I recently implemented something similar in tidybayes (the multiple-orientations-one geom part, not the part that chooses orientation automatically --- I love the idea of having that sniffed out automatically and am planning to add it now too, with an API consistent with whatever is decided here).

I came to a similar solution as @thomasp85 : have a parameter like direction (I called mine orientation) and then write the code to use either x or y based columns depending on that parameter. I did do things a little differently from there: I created a somewhat hackish function define_orientation_variables that I can call at the top of any function that needs to interact with the data. This allowed me to rewrite my geoms very easily; code that looked like this:

draw_panel = function(self, data, panel_params, coord) {
  ...
  data$y = data$ymin + justification * data$height
  ...
}

Became:

draw_panel = function(self, data, panel_params, coord, orientation) {
  define_orientation_variables(orientation)
  ...
  data[[y]] = data[[ymin]] + justification * data[[height]]
  ...
}

Which saved me a bunch of switches and code duplication and other headaches when rewriting those geoms. Now I just write the geoms as if they are have a horizontal orientation in my mind and define_orientation_variables takes care of the vertical case. On the other hand, this is perhaps a very weird style of API that produces code that works for me internally in one package and might not be appropriate for a broadly-used API like ggplot, but I wonder if something similar in spirit could be used here to make building such geoms similarly straightforward.

A few other things:

  • I like the idea of having a parameter to control this explicitly with default value NA meaning "automatic", as @thomasp85 suggested. FWIW as a single data point about discoverability, that's the interface I had assumed it would take after hearing about it on Twitter but before seeing the code.
  • I think that "orientation" is a better word for this than "direction". "direction" connotes more... well, direction to me, like "up" or "down", not "horizontal" or "vertical", which I think of as orientations. This answer on English stack exchange seems to agree with my intuition. I think not using "direction" here would also be more consistent with the use of "direction" elsewhere in the ggplot API (e.g. the direction parameter in scale_colour_brewer controls the direction of the color mapping in the palette, not its orientation, which is meaningless).
  • +1 to @hadley's suggestion of a Geom to inherit from. Relatedly, it strikes me that detect_direction (or what have you) would also make more sense as a method instead of a function.

@thomasp85
Copy link
Copy Markdown
Member Author

Thanks for chiming in @mjskay... Some quick thoughts:

  • I don't feel good about your define_orientation_variables() solution. It is way too magicking and I'll usually refrain from creating environment-changing side effects for the benefit of less typing. Some tooling around this is needed (maybe the current switch_position(), maybe something else), but it should not do funny stuff
  • orientation sounds good to me
  • I'm almost 100% certain that this cannot be abstracted away into a virtual Geom subclass that you can just subclass and everything will be fine. It might be doable with the Stat changes, but will require more effort than it is worth IMO

@hadley the "x|y" nomenclature sounds good to me, but will require some thought in how it is implemented. What would be really nice would be to define two separate sets of aesthetics that needs to be defined, e.g. x, ymin, and ymax or y, xmin, and max, instead of x or y, xmin or ymin, and xmax or ymax

@thomasp85
Copy link
Copy Markdown
Member Author

Ok, last piece of the puzzle: stat_summary(). We have decided to rename the arguments from fun.ymin, fun.y, and fun.ymax to fun.min, fun, and fun.max. But, what should we expect the output of fun.data to be? Currently nothing gets enforced, but should we expect the function to output correctly named columns or should we flip it automatically (e.g. expect it to behave as it should unflipped)?

The easiest is to expect the function to output data with y, ymin, and ymax columns even in the case of flipped axes but will this be weird?

@thomasp85
Copy link
Copy Markdown
Member Author

It was really a no-brainer as all of the supplied function returns data in x-orientation format so this is what we continue to support

@thomasp85 thomasp85 changed the title WIP: Removing direction constraint from geoms Removing direction constraint from geoms Sep 19, 2019
@thomasp85
thomasp85 marked this pull request as ready for review September 19, 2019 12:11
@hadley
hadley commented Sep 19, 2019
Copy link
Copy Markdown
Member

@thomasp85 can you give me a few pointers as to where to focus my effort?

@thomasp85
Copy link
Copy Markdown
Member Author

The main logic lies in has_flipped_aes() so checking the assumptions in there would be first priority. Also, documentation of the feature and whether it is understandable

Comment thread R/utilities.r Outdated
Comment thread R/utilities.r
Comment thread R/utilities.r Outdated
Comment thread R/utilities.r Outdated
Comment thread R/utilities.r Outdated
Comment thread R/utilities.r Outdated
@hadley hadley left a comment
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Two small documentation comments, otherwise it looks good to go.

Comment thread R/utilities.r Outdated
Comment thread R/utilities.r Outdated
@thomasp85
Copy link
Copy Markdown
Member Author

Haven’t I provided examples for all options in the section ?

@lock
lock Bot commented Apr 2, 2020
Copy link
Copy Markdown

This old issue has been automatically locked. If you believe you have found a related problem, please file a new issue (with reprex) and link to this issue. https://reprex.tidyverse.org/

@lock lock Bot locked and limited conversation to collaborators Apr 2, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

9 participants

0