Skip to content

Split geom_hline into geom and annotation (fixes #426) #482

Closed
wants to merge 11 commits into from

3 participants

@wch
Collaborator
wch commented Apr 12, 2012

This fixes #426. As discussed, it splits geom_hline into a geom and annotation.

If this looks like the right approach, I can do the same for vline and abline. If so, should I put annotate_hline, annotate_vline, annotate_abline all in one file since they're quite simple? Or they could be added to their respective geom-Xline.r files.

Also, some of the visual tests will have to be updated.

@wch
Collaborator
wch commented Apr 12, 2012

It also looks like there's a lot of code in the stat_Xline functions that's intended to remove duplicate lines. Can I remove that stuff? It would simplify the code a lot.

Also, stat_abline gives default values for slope and intercept. Is that necessary?

@wch
Collaborator
wch commented Apr 12, 2012

The code for the line stats is a bit simpler with the duplicate stuff removed. It might be a good idea to print a message when using a geom with fixed intercept values, though, because it's easy to do accidentally. I also removed the default values for StatAbline.

Here's some code that tests what should be expected when the duplicate line prevention code is removed:

p <- ggplot(mtcars, aes(x = wt, y = mpg)) + geom_point()

p + geom_abline(aes(intercept = mpg, slope = mpg/5), alpha = .2) # Lots of lines
p + geom_abline(intercept = 20, slope = 1, alpha = .2)      # Should be dark
p + geom_abline(aes(intercept = 20, slope = 1), alpha = .2) # Should be dark
p + annotate_abline(intercept = 20, slope = 1, alpha = .2)  # Should be light


p + geom_vline(aes(xintercept = wt), alpha=.2)   # Lines through each point
p + geom_vline(xintercept = 3, alpha=.2)         # Should be dark
p + geom_vline(aes(xintercept = 3), alpha=.2)    # Should be dark
p + annotate_vline(xintercept = 3, alpha=.2)     # Should be light


p + geom_hline(aes(yintercept = mpg), alpha=.2)   # Lines through each point
p + geom_hline(yintercept = 20, alpha=.2)         # Should be dark
p + geom_hline(aes(yintercept = 20), alpha=.2)    # Should be dark
p + annotate_hline(yintercept = 20, alpha=.2)     # Should be light
@hadley
Owner
hadley commented Apr 12, 2012

Looks good! I wonder now if the stat functions could be removed altogether and just moved into the geoms? I'm trying to remember why I had separate stat functions - it's probably something to do with either correctly setting scale limits, or with non-Cartesian coordinates. If you have tests for those and they work fine, I think we could go ahead and remove the stat functions.

@hadley
Owner
hadley commented Apr 12, 2012

And we might not want to include this in 0.9.1 since it breaks backward compatability

@hadley hadley and 1 other commented on an outdated diff Apr 12, 2012
R/annotate-lines.r
@@ -0,0 +1,41 @@
+#' @rdname geom_hline
+#' @export
+annotate_hline <- function(yintercept = NULL, ...) {
@hadley
Owner
hadley added a note Apr 12, 2012

To reduce duplication (esp. with annotate) maybe the the layer construction should be extracted out into its own function?

@hadley
Owner
hadley added a note Apr 12, 2012

Or should annotate have special cases for vline, hline, and abline so that you can just use annotate?

@wch
Collaborator
wch added a note Apr 12, 2012

I was thinking that one way to go would be to simply call annotation(), but I think it would require some modification. Is there a reason why annotation() looks specifically for x, xmin, and xmax, but not other values, like xintercept?

@hadley
Owner
hadley added a note Apr 12, 2012

I probably just forgot xintercept and yintercept - the other problem is that they don't currently work with stat_identity, but maybe that can go away as per my other comment. The reason that position aesthetics are treated differently is that they should be scaled (i.e. you specify position in data coordinates), but all other aesthetics should be treated as set parameter values (e.g. colour = "red" shouldn't be scaled).

That said, there were a couple of bug reports on the mailing list about annotations not working with various scales.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@hadley hadley commented on an outdated diff Apr 12, 2012
R/geom-abline.r
@@ -1,17 +1,19 @@
#' Line specified by slope and intercept.
#'
-#' The abline geom adds a line with specified slope and intercept to the
-#' plot.
@hadley
Owner
hadley added a note Apr 12, 2012

I wonder if geom_vline, geom_hline and geom_abline should all be documented in one function?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@hadley hadley commented on the diff Apr 12, 2012
R/geom-hline.r
#'
#' # With coordinate transforms
-#' p + geom_hline(aes(yintercept=mpg)) + coord_equal()
-#' p + geom_hline(aes(yintercept=mpg)) + coord_flip()
-#' p + geom_hline(aes(yintercept=mpg)) + coord_polar()
+#' p + geom_hline(aes(yintercept = mpg)) + coord_equal()
@hadley
Owner
hadley added a note Apr 12, 2012

Could we eliminate some of these given that we now have the visual tests?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@wch
Collaborator
wch commented Apr 12, 2012

Regarding removing the stat: it apparently also can work if passed a function for intercept. I think if that capability were removed, it should be simple to move the remaining code into the geom.

@hadley
Owner
hadley commented Apr 12, 2012

I now think that should be removed - it's not elegant, and it's not inline with the way that the rest of ggplot2 works.

@wch
Collaborator
wch commented Apr 12, 2012

OK, I removed the stat and the examples I posted above still work. The code is much simpler now.

What remains to (possibly) be done:

  • Merge help pages
  • Move annotate_Xline functions into annotate()
@wch
Collaborator
wch commented Apr 12, 2012

Another thought: as long as we're breaking compatibility, all these geoms could probably be collapsed into a single one, like geom_line.

It could take xintercept, yintercept, and slope. The xintercept and yintercept would be incompatible, but you could use slope with either x or y intercept.

@hadley
Owner
hadley commented Apr 12, 2012

I deliberately made it three separate functions because I think it makes more sense to keep behaviours separate, especially when you start having combinations of arguments that don't make sense.

@hadley hadley commented on an outdated diff Apr 12, 2012
R/geom-hline.r
ranges <- coord_range(coordinates, scales)
+ if (!is.null(yintercept))
@hadley
Owner
hadley added a note Apr 12, 2012

I'd prefer data$y <- yintercept %||% data$yintercept

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@BrianDiggs

So a horizontal line would be specified with yintercept=... and slope=0, a vertical line with xintercept=... and slope=Inf? Could the value of slope be inferred if missing and only one of xintercept or yintercept is specified (the assumption being if no slope is specified, a horizontal or vertical line is desired)?

@wch
Collaborator
wch commented Apr 12, 2012

@BrianDiggs I was thinking of having slope default to NULL, and infer whether it's horizontal or vertical by yintercept or xintercept is present.

Come to think of it, you can always specify a line with two of the three: xintercept, yintercept, and slope. And if you just have xintercept or yintercept, you can assume that it's vertical or horizontal.

Here's what combinations would be valid:

xintercept yintercept slope valid?
yes yes yes no
yes yes no yes
yes no yes yes
yes no no yes
no yes yes yes
no yes no yes
no no yes no
no no no no
@hadley
Owner
hadley commented Apr 12, 2012

Yes, this is the way that abline works in base R - and I don't like it. If you need a table to figure out what the valid combination of arguments is, I think there's something wrong with your function!

@wch
Collaborator
wch commented Apr 13, 2012

I've consolidated the docs into geom-hline.r, and clarified them a bit.

There's one inconsistency between annotate form of abline and the others. With hline and vline, you can pass a vector as the intercept values, but with abline, you can't:

# Works
p + annotate("hline", yintercept = seq(10, 30, by = 5))
p + annotate("vline", xintercept = 1:5)

# Doesn't work
p + annotate("abline", intercept = c(17, 22), slope = c(0.5, 1))

I think this is because annotate() captures xintercept and yintercept explicitly and puts them in a list that serves as the data passed to geom_xxx, but it doesn't capture intercept and slope, so they just get passed as ... args, which are expected to be single values.

In general, I like the idea of being able to pass vectors as aesthetic properties using annotate -- then it could be used to draw any sort of geom based on vector values. This may require some retooling of how it works, though.

@hadley
Owner
hadley commented Apr 13, 2012

Oh hmmm, good catch. The problem in general is that putting other aesthetics in the data frame will cause them to be mapped - maybe we need some way to turn that off? (But that seems like it would add a lot of complexity for little gain)

@hadley hadley closed this Apr 13, 2012
@hadley hadley reopened this Apr 13, 2012
@wch
Collaborator
wch commented Apr 13, 2012

Could we just rep all the passed-in vectors to be the same length as the longest vector, and then put them all into a data frame? That way, none of the aesthetics would have to hard-coded into annotate(). Are values for ... used for anything else besides aesthetic properties?

@hadley
Owner
hadley commented Apr 13, 2012

But how would you stop them from being mapped? i.e. if you do annotate("text", label = "My label", colour = "blue") you don't want the scale to map blue to something else.

@hadley
Owner
hadley commented Apr 13, 2012

The bug with geom_abline and multiple slopes/intercepts is probably related to #31

@wch
Collaborator
wch commented Apr 16, 2012

Regarding stopping vars from being mapped, I can't see a way to avoid it right now. What if there were an option to pass a "raw" (unmapped) data frame to layer()?

@hadley
Owner
hadley commented Apr 16, 2012

I think that's something I explored a bit in the layers repo - lets not worry about it for now.

@hadley
Owner
hadley commented Feb 24, 2014

Could you please rebase/merge against master, re-document with the development version of roxygen2 (install_github("klutometis/roxygen) and resubmit?

@hadley hadley closed this Feb 24, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.