add length argument to geom_rug to allow different lengths #3109

daniel-wells · 2019-01-29T13:49:48Z

library(ggplot2)

# current behaviour with default rug length
ggplot(airquality,
       aes(x = Ozone,
           y = Solar.R)) + 
  geom_point() +
  geom_rug()
#> Warning: Removed 42 rows containing missing values (geom_point).

# with smaller rug lengths
ggplot(airquality,
       aes(x = Ozone,
           y = Solar.R)) + 
  geom_point() +
  geom_rug(length=0.015)
#> Warning: Removed 42 rows containing missing values (geom_point).

# with larger rug lengths
ggplot(airquality,
       aes(x = Ozone,
           y = Solar.R)) + 
  geom_point() +
  geom_rug(length=0.05) +
  scale_x_continuous(expand=c(0.1,0.1)) +
  scale_y_continuous(expand=c(0.1,0.1))
#> Warning: Removed 42 rows containing missing values (geom_point).

^{Created on 2019-01-29 by the reprex package (v0.2.1)}

yutannihilation · 2019-02-20T03:56:24Z

This was once addressed in #662, which was not merged (due to the development bandwidth shortage for reviewing?). Comparing with that, I have two questions:

Is length the best name for the parameter? (width, or size?)
Should the value be specified with or without the unit? (3 or unit(0.3, "npc")?) In other places, the unitless numbers are interpreted as .pt.

clauswilke · 2019-02-20T04:03:20Z

I think rug length should be specified in units, just like axis tick length. Otherwise, rug length will depend on the chosen coordinate system, and that is awkward. In fact, we see in the example figures that horizontal and vertical rug lines have different lengths. I realize this is how the current code behaves, but I think it can be done better, or at least we can provide the option to do better, even if we leave the current default in npc units.

yutannihilation · 2019-02-21T23:54:05Z

I think rug length should be specified in units, just like axis tick length. Otherwise, rug length will depend on the chosen coordinate system, and that is awkward.

I agree. Specifying in units gives us more flexibility.

daniel-wells · 2019-02-24T02:13:32Z

Thanks for the feedback!

I have changed the length specification into units and added a test based on the one in #662

I chose length for the name as size is currently used to specify the thickness, and width sounds like it should control the thickness for the vertical bottom or top rugs. Looking at #662 we could use rugsize or ruglength instead?

library(ggplot2)

ggplot(airquality,
       aes(x = Ozone,
           y = Solar.R)) + 
  geom_point() +
  geom_rug(length = unit(10,"pt"))
#> Warning: Removed 42 rows containing missing values (geom_point).

^{Created on 2019-02-24 by the reprex package (v0.2.1)}

yutannihilation · 2019-02-24T12:18:32Z

I chose length for the name as size is currently used to specify the thickness, and width sounds like it should control the thickness for the vertical bottom or top rugs.

Thanks, then it sounds reasonable to choose length. (Sorry, I forgot we already use length as in axis.ticks.length argument of theme()).

yutannihilation

Generally, looks good to me. I added some minor comments.

yutannihilation · 2019-02-26T04:21:54Z

R/geom-rug.r

+#' # increase the line length and
+#' # expand axis to avoid overplotting
+#' p + geom_rug(length = unit(0.05,"npc")) +
+#'    scale_y_continuous(expand=c(0.1,0.1))


Could you follow the tidyverse style guide? You need more spaces here.

tests/testthat/test-geom-rug.R

yutannihilation · 2019-02-26T04:25:36Z

NEWS.md

@@ -1,5 +1,7 @@
 # ggplot2 3.1.0.9000

+* `geom_rug()` gains a "length" option to allow for changing the length of the rug lines. (@daniel-wells, #3109)


"length" should be length.

yutannihilation · 2019-02-26T04:26:53Z

R/geom-rug.r

@@ -76,7 +80,10 @@ geom_rug <- function(mapping = NULL, data = NULL,
 GeomRug <- ggproto("GeomRug", Geom,
  optional_aes = c("x", "y"),

-  draw_panel = function(data, panel_params, coord, sides = "bl", outside) {
+  draw_panel = function(data, panel_params, coord, sides = "bl", length, outside) {
+    if (!is(length, "unit")) {


You can use inherits() for S3 classes.

yutannihilation · 2019-02-26T04:30:22Z

R/geom-rug.r

-  draw_panel = function(data, panel_params, coord, sides = "bl", outside) {
+  draw_panel = function(data, panel_params, coord, sides = "bl", length, outside) {
+    if (!is(length, "unit")) {
+      stop("'length' must be a 'unit' object.")


Please add call. = FALSE to suppress verbose tracebacks. c.f https://style.tidyverse.org/error-messages.html

daniel-wells · 2019-02-28T03:15:25Z

Thanks for the review! I have incorporated the suggested improvements :)

yutannihilation

Thanks! This looks ready to merge now.

yutannihilation · 2019-02-28T04:05:59Z

@clauswilke Do you have any more concerns or comments? Though I think this is safe to merge since the changes are not so huge, I'll wait if you need some time to look the code more detailedly (I know you are busy).

clauswilke · 2019-02-28T04:29:56Z

R/geom-rug.r

@@ -76,7 +80,10 @@ geom_rug <- function(mapping = NULL, data = NULL,
 GeomRug <- ggproto("GeomRug", Geom,
  optional_aes = c("x", "y"),

-  draw_panel = function(data, panel_params, coord, sides = "bl", outside) {
+  draw_panel = function(data, panel_params, coord, sides = "bl", length, outside) {


It's generally better to add new arguments (here, length) after previously existing arguments, in case some downstream code depends on the order of the arguments. Also, better to provide a default length here (and maybe also add a default outside value).

Should I change the ordering in the geom_rug function as well as, or instead of, draw_panel? It seems changing the order or defaults for draw_panel makes no difference when adding geom_rug() to a plot.

No. In geom_rug(), length is placed after ..., which means the user need to pass the argument with its name. So, there's no chance for length to be mistaken. But, in draw_panel(), outside can be specified without the name. If some existing code uses outside positionally, it might fail due to this change.

Does this answer your question?

(Sorry that I didn't notice this on my review...)

Ah I see thanks! I've implemented the three new suggestions.

clauswilke · 2019-02-28T04:37:01Z

R/geom-rug.r

@@ -4,17 +4,14 @@
 #' with the two 1d marginal distributions. Rug plots display individual
 #' cases so are best used with smaller datasets.
 #'
-#' The rug lines are drawn with a fixed size (3\% of the total plot size) so
-#' are dependent on the overall scale expansion in order not to overplot
-#' existing data.


I think the deleted lines 7-9 contain important information and should be kept. But edit so it's clear that the default size can be modified.

Example:

By default, the rug lines are drawn with a length that corresponds to 3% of the total plot size. Since the default scale expansion of for continuous variables is 5% at both ends of the scale, the rug will not overlap with any data points under the default settings.

clauswilke · 2019-02-28T04:38:08Z

Looks good to me after the two comments I made have been addressed. I don't entirely understand how the regression tests work but I trust you've thought them through.

yutannihilation · 2019-02-28T05:06:38Z

Thanks!

I don't entirely understand how the regression tests work but I trust you've thought them through.

The current master only checks the numbers of rug lines on a normal plot and on the flipped version. This PR adds tests about the validation on length and the actual length of rug lines. We might be able to check things more thoroughly, but I feel it's enough.

yutannihilation · 2019-02-28T05:12:06Z

tests/testthat/test-geom-rug.R

+
+test_that("Rug length needs unit object", {
+  p <- ggplot(df, aes(x,y))
+  expect_is(p + geom_rug(length=grid::unit(0.01, "npc")), "ggplot")


Sorry, one more comment here. This expect_is() seems no more needed. If geom_rug() raises an error with a unit object, the test case below will fail so that we can notice something is wrong.

yutannihilation

Thanks, a few more changes are needed.

yutannihilation · 2019-03-01T23:52:22Z

R/geom-rug.r

-#' The rug lines are drawn with a fixed size (3\% of the total plot size) so
-#' are dependent on the overall scale expansion in order not to overplot
-#' existing data.
+#' By default, the rug lines are drawn with a length that corresponds to 3%


You need to escape this % like the current one:

ggplot2/R/geom-rug.r

Line 7 in 9df0a07

#' The rug lines are drawn with a fixed size (3\% of the total plot size) so

Oops sorry! corrected

yutannihilation · 2019-03-01T23:54:38Z

R/geom-rug.r

-#' existing data.
+#' By default, the rug lines are drawn with a length that corresponds to 3%
+#' of the total plot size. Since the default scale expansion of for continuous
+#' variables is 5% at both ends of the scale, the rug will not overlap with


yutannihilation · 2019-03-04T22:30:24Z

Perfect, thanks!

daniel-wells · 2019-03-05T00:51:36Z

Awesome! Thanks for all your feedback and help :)

lock · 2019-09-01T01:08:57Z

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/

daniel-wells added 3 commits January 29, 2019 13:46

add length argument to geom_rug to allow different lengths

de51017

add PR number

c861d83

use spaces not tabs

7c7db06

daniel-wells added 3 commits February 24, 2019 02:04

move length unit specification to function parameter level

9caa182

test that geom_rug length parameter needs unit object

017b5ed

Merge branch 'master' into geom-rug-length

f14492f

daniel-wells added 2 commits February 24, 2019 10:32

update geom_rug example with unit length

1f9c124

update geom_rug documentation

628db16

yutannihilation reviewed Feb 26, 2019

View reviewed changes

daniel-wells added 5 commits February 28, 2019 02:33

add spaces for tidyverse style

8f952f5

use code quotes

e8919eb

use inherits for S3 class check

f867694

use call. = F

4ddc907

add geom_rug tests for actual lengths of grob

9149838

yutannihilation approved these changes Feb 28, 2019

View reviewed changes

clauswilke reviewed Feb 28, 2019

View reviewed changes

yutannihilation reviewed Feb 28, 2019

View reviewed changes

daniel-wells added 3 commits February 28, 2019 10:32

remove unnecessary test

da35c26

add explanation of rug length default

ec86925

add new parameters last with default value

05237a8

yutannihilation reviewed Mar 1, 2019

View reviewed changes

escape %

0544d57

yutannihilation merged commit 9ced958 into tidyverse:master Mar 4, 2019

paleolimbot mentioned this pull request May 2, 2019

Improve theme documentation #3294

Closed

lock bot locked and limited conversation to collaborators Sep 1, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add length argument to geom_rug to allow different lengths #3109

add length argument to geom_rug to allow different lengths #3109

daniel-wells commented Jan 29, 2019

yutannihilation commented Feb 20, 2019

clauswilke commented Feb 20, 2019

yutannihilation commented Feb 21, 2019

daniel-wells commented Feb 24, 2019

yutannihilation commented Feb 24, 2019

yutannihilation left a comment

yutannihilation Feb 26, 2019

yutannihilation Feb 26, 2019

yutannihilation Feb 26, 2019

yutannihilation Feb 26, 2019

daniel-wells commented Feb 28, 2019

yutannihilation left a comment

yutannihilation commented Feb 28, 2019 •

edited

Loading

clauswilke Feb 28, 2019

daniel-wells Feb 28, 2019

yutannihilation Feb 28, 2019

daniel-wells Feb 28, 2019

clauswilke Feb 28, 2019

clauswilke commented Feb 28, 2019

yutannihilation commented Feb 28, 2019

yutannihilation Feb 28, 2019

yutannihilation left a comment

yutannihilation Mar 1, 2019

daniel-wells Mar 2, 2019

yutannihilation Mar 1, 2019

yutannihilation commented Mar 4, 2019

daniel-wells commented Mar 5, 2019

lock bot commented Sep 1, 2019

		@@ -1,5 +1,7 @@
		# ggplot2 3.1.0.9000

		* `geom_rug()` gains a "length" option to allow for changing the length of the rug lines. (@daniel-wells, #3109)

add length argument to geom_rug to allow different lengths #3109

add length argument to geom_rug to allow different lengths #3109

Conversation

daniel-wells commented Jan 29, 2019

yutannihilation commented Feb 20, 2019

clauswilke commented Feb 20, 2019

yutannihilation commented Feb 21, 2019

daniel-wells commented Feb 24, 2019

yutannihilation commented Feb 24, 2019

yutannihilation left a 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

daniel-wells commented Feb 28, 2019

yutannihilation left a comment

Choose a reason for hiding this comment

yutannihilation commented Feb 28, 2019 • edited Loading

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

clauswilke commented Feb 28, 2019

yutannihilation commented Feb 28, 2019

Choose a reason for hiding this comment

yutannihilation left a 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

yutannihilation commented Mar 4, 2019

daniel-wells commented Mar 5, 2019

lock bot commented Sep 1, 2019

yutannihilation commented Feb 28, 2019 •

edited

Loading