Skip to content

add param to alter width of rug segments in geom_rug #662

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

Closed
wants to merge 5 commits into from

Conversation

jrnold
Copy link
Contributor

@jrnold jrnold commented Sep 10, 2012

I noticed that the width of the segments in geom_rug was hardcoded as 0.03. I added a parameter to allow the user to change it. But, I'm not sure that is the most idiomatic way to do it. Perhaps it should just use the width aes.

@@ -16,20 +19,20 @@
#' p + geom_point() + geom_rug(sides="trbl") # All four sides
#' p + geom_point() + geom_rug(position='jitter')
geom_rug <- function (mapping = NULL, data = NULL, stat = "identity", position = "identity", sides = "bl", ...) {
Copy link
Member

Choose a reason for hiding this comment

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

rugwidth should be here, in the definition for geom_rug, instead of in GeomRug$new.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So it doesn't need to appear in both locations? rugwidth will be passed to GeomRug$draw in the triple-dots, and there is no need for a default? I was going off how geom_boxplot was written.

Copy link
Member

Choose a reason for hiding this comment

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

It should be something like:

geom_rug <- function (mapping = NULL, data = NULL, stat = "identity", position = "identity", sides = "bl", rugwidth = 0.03, ...) {
  GeomRug$new(mapping = mapping, data = data, stat = stat, position = position, sides = sides, rugwidth = rugwidth, ...)
}

@@ -15,21 +18,21 @@
#' p + geom_point() + geom_rug(sides="b") # Rug on bottom only
#' p + geom_point() + geom_rug(sides="trbl") # All four sides
#' p + geom_point() + geom_rug(position='jitter')
geom_rug <- function (mapping = NULL, data = NULL, stat = "identity", position = "identity", sides = "bl", ...) {
GeomRug$new(mapping = mapping, data = data, stat = stat, position = position, sides = sides, ...)
geom_rug <- function (mapping = NULL, data = NULL, stat = "identity", position = "identity", sides = "bl", rugwidth=0.03 ...) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

okay, fixed now. Wow, i'm sorry. That original line was really sloppy of me.

@wch
Copy link
Member

wch commented Sep 14, 2012

It might be better if it would accept a unit object, so that the length could be specified in absolute or relative lengths. The default could still be unit(0.03, "npc").

Also, for future reference: there are a lot of spurious whitespace changes in the diff - better not to have those.

@jrnold
Copy link
Contributor Author

jrnold commented Sep 15, 2012

Good idea. I'll change it to units().

Sorry about the whitespace. I have my text editor set to kill all trailing whitespace. I'll make sure that those whitespace changes don't happen again.

@jrnold
Copy link
Contributor Author

jrnold commented Sep 15, 2012

Okay, I now have rugwidth accepting units arguments. How do you think it should handle the case, if the user does not pass it a unit object?

@wch
Copy link
Member

wch commented Sep 24, 2012

I think it should only work with unit objects. There are other places where unit objects are required...

@jrnold
Copy link
Contributor Author

jrnold commented Sep 24, 2012

So should I check that rugwith has class unit, and stop if it is not? Or try to coerce the argument into a unit object?

@wch
Copy link
Member

wch commented Sep 24, 2012

I think that it would be good to check that it's a unit, and give an error if it's not.

@jrnold
Copy link
Contributor Author

jrnold commented Sep 24, 2012

I added a check on the class of the rugwidth argument and a corresponding unit test.

@wch
Copy link
Member

wch commented Sep 25, 2012

Looks good! Just FYI, it'll probably be a while before this is merged, since the next release, 0.9.3, will be bugfixes. After that, ggplot2 will be converted from proto to S3, and I think that will be the time to add features like this.

@jrnold
Copy link
Contributor Author

jrnold commented Sep 25, 2012

no problem; I'm in no rush. I had noticed this while tweaking the geom to produce Tufte's range frames, and thought it would be a good idea to push it upstream.

@hadley
Copy link
Member

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
@lock
Copy link

lock bot commented Jan 19, 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 Jan 19, 2019
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.

3 participants