Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add length argument to geom_rug to allow different lengths #3109

Merged
merged 17 commits into from Mar 4, 2019

Conversation

@daniel-wells
Copy link
Contributor

@daniel-wells daniel-wells commented Jan 29, 2019

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
Copy link
Member

@yutannihilation yutannihilation commented Feb 20, 2019

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
Copy link
Member

@clauswilke clauswilke commented Feb 20, 2019

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
Copy link
Member

@yutannihilation yutannihilation commented Feb 21, 2019

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
Copy link
Contributor Author

@daniel-wells daniel-wells commented Feb 24, 2019

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
Copy link
Member

@yutannihilation yutannihilation commented Feb 24, 2019

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()).

Copy link
Member

@yutannihilation yutannihilation left a comment

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

R/geom-rug.r Outdated
#' # 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))
Copy link
Member

@yutannihilation yutannihilation Feb 26, 2019

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

tests/testthat/test-geom-rug.R Outdated Show resolved Hide resolved
NEWS.md Outdated
@@ -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)
Copy link
Member

@yutannihilation yutannihilation Feb 26, 2019

"length" should be length.

R/geom-rug.r Outdated
@@ -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")) {
Copy link
Member

@yutannihilation yutannihilation Feb 26, 2019

You can use inherits() for S3 classes.

R/geom-rug.r Outdated
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.")
Copy link
Member

@yutannihilation yutannihilation Feb 26, 2019

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

@daniel-wells
Copy link
Contributor Author

@daniel-wells daniel-wells commented Feb 28, 2019

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

Copy link
Member

@yutannihilation yutannihilation left a comment

Thanks! This looks ready to merge now.

@yutannihilation
Copy link
Member

@yutannihilation yutannihilation commented Feb 28, 2019

@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).

R/geom-rug.r Outdated
@@ -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) {
Copy link
Member

@clauswilke clauswilke Feb 28, 2019

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).

Copy link
Contributor Author

@daniel-wells daniel-wells Feb 28, 2019

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.

Copy link
Member

@yutannihilation yutannihilation Feb 28, 2019

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...)

Copy link
Contributor Author

@daniel-wells daniel-wells Feb 28, 2019

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

@@ -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.
Copy link
Member

@clauswilke clauswilke Feb 28, 2019

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
Copy link
Member

@clauswilke clauswilke commented Feb 28, 2019

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
Copy link
Member

@yutannihilation yutannihilation commented Feb 28, 2019

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.


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")
Copy link
Member

@yutannihilation yutannihilation Feb 28, 2019

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.

Copy link
Member

@yutannihilation yutannihilation left a comment

Thanks, a few more changes are needed.

R/geom-rug.r Outdated
#' 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%
Copy link
Member

@yutannihilation yutannihilation Mar 1, 2019

You need to escape this % like the current one:

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

Copy link
Contributor Author

@daniel-wells daniel-wells Mar 2, 2019

Oops sorry! corrected

R/geom-rug.r Outdated
#' 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
Copy link
Member

@yutannihilation yutannihilation Mar 1, 2019

ditto

@yutannihilation
Copy link
Member

@yutannihilation yutannihilation commented Mar 4, 2019

Perfect, thanks!

@yutannihilation yutannihilation merged commit 9ced958 into tidyverse:master Mar 4, 2019
4 checks passed
@daniel-wells
Copy link
Contributor Author

@daniel-wells daniel-wells commented Mar 5, 2019

Awesome! Thanks for all your feedback and help :)

@lock
Copy link

@lock lock bot commented Sep 1, 2019

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 Sep 1, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants