-
Notifications
You must be signed in to change notification settings - Fork 2.1k
Rename aesthetics in scales and consistently convert US to British spelling #2750
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is definitely going in the right direction, but I feel like the API needs a bit more thought.
} | ||
|
||
# x is a list of aesthetic mappings, as generated by aes() | ||
rename_aes <- function(x) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where does this get called from?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The main user is the aes()
function, but it's actually called from many places:
Lines 77 to 83 in 17b45f9
aes <- function(x, y, ...) { | |
exprs <- rlang::enquos(x = x, y = y, ...) | |
is_missing <- vapply(exprs, rlang::quo_is_missing, logical(1)) | |
aes <- new_aes(exprs[!is_missing], env = parent.frame()) | |
rename_aes(aes) | |
} |
Line 17 in 528b0ce
g$default_aes <- defaults(rename_aes(new), old) |
Line 57 in 4299917
args <- rename_aes(args) |
Line 86 in c1908f1
mapping <- rename_aes(mapping) |
Line 63 in 3129bf8
args <- rename_aes(args) |
Lines 99 to 100 in 17b45f9
# Split up params between aesthetics, geom, and stat | |
params <- rename_aes(params) |
Line 183 in 17b45f9
override.aes = rename_aes(override.aes), |
R/aes.r
Outdated
duplicated_names_message <- paste0( | ||
"`", duplicated_names, "`", collapse = ", " | ||
) | ||
warning( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wasn't this previously the responsibility of plyr::rename()
? Shouldn't duplicated names also be checked in standardise_aes_names()
? Should we also check for code like aes(colour=1,colour=2,colour=3)
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this code was taken from plyr::rename()
. Because I now have a function that converts names rather than a named list it didn't make sense to call plyr::rename()
anymore. Also, this gives us the opportunity to customize the warning message to be more meaningful.
Whether duplicated names should be checked in standardise_aes_names()
is not clear to me. I think an argument can be made either way. If the job of standardise_aes_names()
is to simply take a vector of strings and standardize each of them then it doesn't necessarily have the job to check for duplicates. Maybe there's a downstream use case where somebody needs aesthetics names standardized without having duplicates checked. On the other hand, moving the check into that function would work just fine with the current code base.
Regarding the case of multiple identical aesthetics, it does get caught by the same check:
> aes(colour=1, colour=2, colour=3)
Aesthetic mapping:
* `colour` -> 1
* `colour` -> 2
* `colour` -> 3
Warning message:
Standardisation of aesthetics has created duplicates for the following: `colour`, `colour`
So maybe it's just a matter of making the error message a little clearer. I think printing out the complete list of final aesthetic names, rather than just the ones that are duplicated, might be helpful.
I have improved the error message for duplicated aesthetics. Most importantly, now each duplicated aesthetic is at most listed once. I tried outputting the entire list of aesthetics but that was confusing. > aes(shape = 1, pch = 2, colour = 1, colour = 2, colour = 3)
Aesthetic mapping:
* `shape` -> 1
* `shape` -> 2
* `colour` -> 1
* `colour` -> 2
* `colour` -> 3
Warning message:
Duplicated aesthetics after name standardisation: shape, colour |
if (length(duplicated_names) > 0L) { | ||
duplicated_message <- paste0(unique(duplicated_names), collapse = ", ") | ||
warning( | ||
"Duplicated aesthetics after name standardisation: ", duplicated_message, call. = FALSE |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we make this an error?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll leave this to your decision. My own philosophy is it's only an error if the software cannot recover. Here, the duplicated aesthetics are simply ignored and a plot is produced:
library(ggplot2)
df <- data.frame(x = 1:10, y = 1:10)
ggplot(df, aes(x, y, shape = "*", pch = "a")) + geom_point()
#> Warning: Duplicated aesthetics after name standardisation: shape
It's not that different from this case, which doesn't even create a warning:
ggplot(df, aes(x, y, shape = "*")) + geom_point(aes(pch = "a"))
Created on 2018-07-13 by the reprex package (v0.2.0).
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/ |
This PR closes #2649. It has two parts: (i) It reworks and simplifies renaming of aesthetic names. (ii) It uses the reworked code in the scale functions, to address issues such as #2674.
Part II is straightforward. Part I needs a few words of explanation.
Most importantly, I deleted code that supposedly does partial matching of aesthetic names:
ggplot2/R/aes.r
Lines 148 to 150 in 5f49fb4
As far as I can tell, this code does not do anything, and certainly does not do partial matching. The reason is that
match()
does not do partial matching:Indeed, in my testing, the current ggplot2 does not do partial matching. It does convert
col
tocolour
, but only because that particular match is listed among the base-to-ggplot conversions:Second, I've exported a new internal function,
standardise_aes_names()
. This function is needed by anybody who wants to implement their own scales and take advantage of aes standardisation.With this patch, the following is possible:
Created on 2018-07-11 by the reprex package (v0.2.0).