rlang/purrr style anonymous function specification in stat_function #3160

dkahle · 2019-02-22T20:14:48Z

Closes #3159.

…tion

…/ggplot2 into purrr-style-stat-function

yutannihilation · 2019-02-22T21:11:07Z

R/ggplot2.r

@@ -3,5 +3,5 @@

 #' @import scales grid gtable
 #' @importFrom stats setNames
-#' @importFrom rlang quo quos
+#' @importFrom rlang quo quos as_function


As you use rlang::, you don't need to import this.

Won't we need it somewhere to avoid CRAN warnings about not being imported? Otherwise, I'm fine removing of course.

I don't think so. I suggest removing. If it's a problem the Travis build will break.

What do you mean? As long as you use fully qualified form (rlang::as_function()), you don't need to import.

clauswilke · 2019-02-22T21:18:49Z

R/stat-function.r

@@ -5,7 +5,7 @@
 #' the x axis, and the results are drawn (by default) with a line.
 #'
 #' @eval rd_aesthetics("stat", "function")
-#' @param fun function to use. Must be vectorised.
+#' @param fun function to use or formula. Must be vectorised.


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

The .x in the formula should also be explained here.

And we also need a link to as_function's doc.

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

tests/testthat/test-stats-function.r

yutannihilation · 2019-02-22T21:37:42Z

R/stat-function.r

@@ -49,6 +49,7 @@
 #' # Using a custom function
 #' test <- function(x) {x ^ 2 + x + 20}
 #' f + stat_function(fun = test)
+#' f + stat_function(fun = ~ .x^2 + .x + 20)


Probably, . is better for single-argument function?

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.

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)

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

yutannihilation · 2019-02-22T21:41:40Z

R/stat-function.r

@@ -5,7 +5,7 @@
 #' the x axis, and the results are drawn (by default) with a line.
 #'
 #' @eval rd_aesthetics("stat", "function")
-#' @param fun function to use. Must be vectorised.
+#' @param fun function to use or formula. Must be vectorised.


And we also need a link to as_function's doc.

dkahle · 2019-02-23T02:14:53Z

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?

clauswilke · 2019-02-23T02:34:30Z

R/stat-function.r

@@ -16,39 +19,44 @@
 #'   \item{x}{x's along a grid}
 #'   \item{y}{value of function evaluated at corresponding x}
 #' }
+#' @seealso \code{\link[rlang:as_function]{as_function()}}


Should this be rlang::as_function? (two colons instead of one)

I thought in the link you put a single colon; works in the docs as I built them, at least.

A single colon is correct.

c.f. https://cran.r-project.org/doc/manuals/R-exts.html#Cross_002dreferences

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

Looks right with parentheses. (I'm seeing more like [rlang::as_function()].) Got it.

Yes, [rlang::as_function()] is correct, sorry. More details are here: https://cran.r-project.org/web/packages/roxygen2/vignettes/markdown.html#links

clauswilke · 2019-02-23T02:37:08Z

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?

clauswilke · 2019-02-23T02:43:13Z

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.

dkahle · 2019-02-23T02:55:40Z

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.

yutannihilation · 2019-02-23T03:23:21Z

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.

yutannihilation · 2019-02-25T05:31:53Z

The R-devel issue was solved by #3163.

yutannihilation · 2019-03-07T13:06:29Z

@dkahle Could you remove as_function from @importFrom? I feel this is ready to merge if you fix it.

yutannihilation · 2019-03-08T01:15:15Z

@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 ~ notations.

yutannihilation · 2019-03-08T01:17:05Z

Thanks!

clauswilke · 2019-03-08T01:20:53Z

@yutannihilation Ok. Maybe open an issue about this so we don't forget?

yutannihilation · 2019-03-08T01:37:04Z

Good idea, thanks. Here it is: #3181

lock · 2019-09-04T02:08:59Z

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/

dkahle added 3 commits February 22, 2019 14:11

allow rlang/purrr style anonymous function specifiations in stat_func…

b393505

…tion

allow rlang/purrr style anonymous function specifiations in stat_func…

e69ab49

…tion (tidyverse#3159)

Merge branch 'purrr-style-stat-function' of https://github.com/dkahle…

34a74d0

…/ggplot2 into purrr-style-stat-function

yutannihilation reviewed Feb 22, 2019

View reviewed changes

clauswilke reviewed Feb 22, 2019

View reviewed changes

tests/testthat/test-stats-function.r Outdated Show resolved Hide resolved

yutannihilation reviewed Feb 22, 2019

View reviewed changes

dkahle added 2 commits February 22, 2019 18:32

unparen ret line in test

0e1a56f

update stat-function docs: fun argument, links, and examples

612b6d1

clauswilke reviewed Feb 23, 2019

View reviewed changes

stat_function: thin a few examples

eb98c89

stat_function: use [rlang::as_function()] to reference lambda syntax

d63d50e

remove @importFrom rlang as_function

2e6d3cc

yutannihilation approved these changes Mar 8, 2019

View reviewed changes

yutannihilation merged commit ae1c8f0 into tidyverse:master Mar 8, 2019

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

rlang/purrr style anonymous function specification in stat_function #3160

rlang/purrr style anonymous function specification in stat_function #3160

Uh oh!

Conversation

dkahle commented Feb 22, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yutannihilation Feb 23, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dkahle commented Feb 23, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

clauswilke commented Feb 23, 2019

Uh oh!

clauswilke commented Feb 23, 2019

Uh oh!

dkahle commented Feb 23, 2019

Uh oh!

yutannihilation commented Feb 23, 2019

Uh oh!

yutannihilation commented Feb 25, 2019

Uh oh!

yutannihilation commented Mar 7, 2019

Uh oh!

yutannihilation commented Mar 8, 2019

Uh oh!

yutannihilation commented Mar 8, 2019

Uh oh!

clauswilke commented Mar 8, 2019

Uh oh!

yutannihilation commented Mar 8, 2019

Uh oh!

lock bot commented Sep 4, 2019

Uh oh!

Uh oh!

yutannihilation Feb 23, 2019 •

edited

Loading