rlang/purrr style anonymous function specification in stat_function#3160
Conversation
…/ggplot2 into purrr-style-stat-function
| #' @import scales grid gtable | ||
| #' @importFrom stats setNames | ||
| #' @importFrom rlang quo quos | ||
| #' @importFrom rlang quo quos as_function |
There was a problem hiding this comment.
As you use rlang::, you don't need to import this.
There was a problem hiding this comment.
Won't we need it somewhere to avoid CRAN warnings about not being imported? Otherwise, I'm fine removing of course.
There was a problem hiding this comment.
I don't think so. I suggest removing. If it's a problem the Travis build will break.
There was a problem hiding this comment.
What do you mean? As long as you use fully qualified form (rlang::as_function()), you don't need to import.
| #' | ||
| #' @eval rd_aesthetics("stat", "function") | ||
| #' @param fun function to use. Must be vectorised. | ||
| #' @param fun function to use or formula. Must be vectorised. |
There was a problem hiding this comment.
Please expand this sentence a bit more. Now that we have two different possible inputs, the documentation needs a little more care. Also, please add a simple example to the examples, or maybe better rewrite one or two of the current ones (so we don't increase the total number of examples).
There was a problem hiding this comment.
The .x in the formula should also be explained here.
There was a problem hiding this comment.
And we also need a link to as_function's doc.
There was a problem hiding this comment.
Happy to update the docs for the argument and update the examples more fully, if you'd like, I was just trying to be as minimalist as possible. Want me to use . or .x for the formula stuff? (@yutannihilation brought the . vs .x part up in another comment, too.)
| #' # Using a custom function | ||
| #' test <- function(x) {x ^ 2 + x + 20} | ||
| #' f + stat_function(fun = test) | ||
| #' f + stat_function(fun = ~ .x^2 + .x + 20) |
There was a problem hiding this comment.
Probably, . is better for single-argument function?
There was a problem hiding this comment.
I thought about that, too: .x seems to be more tidyverse consistent in that it's more referentially transparent in a piping context, but piping is not native to ggplot2. I don't have a strong preference either way.
There was a problem hiding this comment.
I have a slight preference for .x, because I think it imposes slightly lower mental load, in particular for complex formulas. Compare:
fun = ~ (5*.^2 + 3*.) * exp(-2*.^2 + 3*. + 2)
fun = ~ (5*.x^2 + 3*.x) * exp(-2*.x^2 + 3*.x + 2)There was a problem hiding this comment.
OK, I prefer . because it's short, but it's just my preference and I don't have strong opinion. Let's use .x.
(Note that in some places e.g. sec_axis() .x is not allowed, but it's just implementational details.)
| #' | ||
| #' @eval rd_aesthetics("stat", "function") | ||
| #' @param fun function to use. Must be vectorised. | ||
| #' @param fun function to use or formula. Must be vectorised. |
There was a problem hiding this comment.
And we also need a link to as_function's doc.
|
I think I've included the updates in some new commits (dkahle/ggplot2@0e1a56f and dkahle/ggplot2@612b6d1), how can I add those to this PR? Or, do I need to submit a new PR with all of them? |
| #' \item{x}{x's along a grid} | ||
| #' \item{y}{value of function evaluated at corresponding x} | ||
| #' } | ||
| #' @seealso \code{\link[rlang:as_function]{as_function()}} |
There was a problem hiding this comment.
Should this be rlang::as_function? (two colons instead of one)
There was a problem hiding this comment.
I thought in the link you put a single colon; works in the docs as I built them, at least.
There was a problem hiding this comment.
A single colon is correct.
c.f. https://cran.r-project.org/doc/manuals/R-exts.html#Cross_002dreferences
There was a problem hiding this comment.
Sorry, I was not accurate above. A single colon is correct as a raw Rd code, but we should use Markdown-style as we do so in other places: [rlang::as_function].
There was a problem hiding this comment.
Looks right with parentheses. (I'm seeing more like [rlang::as_function()].) Got it.
There was a problem hiding this comment.
Yes, [rlang::as_function()] is correct, sorry. More details are here: https://cran.r-project.org/web/packages/roxygen2/vignettes/markdown.html#links
|
The new commits get added automatically. Note that the travis build fails with R devel. Not exactly sure from a quick glance what the problem is. Could you check if you can identify the issue? |
|
One more issue from my side: Maybe remove 2 or 3 of the examples. Examples are somewhat expensive, because they all get executed on every CRAN check. The current version has 7 plots, which is already a lot. |
|
Cleaned the docs a bit. Looking into the R-devel issue. It's been flagging on me in some other projects, too, so I'm not sure what's going on there. |
|
It seems R devel is currently broken; I believe #3161 is a harmless change, but it also fails only on r-devel. https://travis-ci.org/tidyverse/ggplot2/jobs/497352631 We might need to look into it, but I think it's irrelevant to this PR. |
|
The R-devel issue was solved by #3163. |
|
@dkahle Could you remove |
|
@clauswilke I'm merging this. The number of examples might still needs to be reduced, but I think we can restructure the examples when ggplot2 gets ready to promote |
|
Thanks! |
|
@yutannihilation Ok. Maybe open an issue about this so we don't forget? |
|
Good idea, thanks. Here it is: #3181 |
|
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/ |
3 participants
Closes #3159.