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.

Sign up for GitHub

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

default formula argument to NULL in geom_smooth() #3307

Merged
merged 14 commits into from
Sep 30, 2019
Merged

default formula argument to NULL in geom_smooth() #3307

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
508e1bc
default formula argument to NULL in geom_smooth(); closes #3205
bfgray3 May 7, 2019
4bb34e3
remove news bullet for now since deadline for release has passed
bfgray3 May 14, 2019
04b3327
handle NULL formulas more carefully in geom_smooth()
bfgray3 May 14, 2019
bf963db
respect input formuals in geom_smooth()
bfgray3 May 14, 2019
c4ce320
add parens around formula on rhs of %||%
bfgray3 May 31, 2019
c9644c3
only set formula once the method is known
bfgray3 Jun 5, 2019
0c2826b
refactor
bfgray3 Jun 14, 2019
5910606
tweak formula message for geom_smooth
bfgray3 Jun 14, 2019
99e9ad2
clean up smoothing message
bfgray3 Jun 16, 2019
68e8a30
merge in upstream master
bfgray3 Jun 16, 2019
b4d92e7
fix backwards compatibility of method = 'auto'
bfgray3 Jun 16, 2019
3aaa53e
code aesthetics
bfgray3 Jun 16, 2019
ca5ba90
Fix styling
thomasp85 Sep 30, 2019
6a97cd6
Merge branch 'master' of https://github.com/tidyverse/ggplot2 into bf…
thomasp85 Sep 30, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion R/geom-smooth.r
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,7 @@ geom_smooth <- function(mapping = NULL, data = NULL,
stat = "smooth", position = "identity",
...,
method = "auto",
formula = y ~ x,
formula = NULL,
se = TRUE,
na.rm = FALSE,
show.legend = NA,
Expand Down
14 changes: 10 additions & 4 deletions R/stat-smooth.r
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,9 @@
#' model that `method = "auto"` would use, then set
#' `method = "gam", formula = y ~ s(x, bs = "cs")`.
#' @param formula Formula to use in smoothing function, eg. `y ~ x`,
#' `y ~ poly(x, 2)`, `y ~ log(x)`
#' `y ~ poly(x, 2)`, `y ~ log(x)`. `NULL` by default, in which case
#' `method = "auto"` implies `formula = y ~ x` when there are fewer than 1,000
#' observations and `formula = y ~ s(x, bs = "cs")` otherwise.
#' @param se Display confidence interval around smooth? (`TRUE` by default, see
#' `level` to control.)
#' @param fullrange Should the fit span the full range of the plot, or just
Expand All @@ -38,7 +40,7 @@ stat_smooth <- function(mapping = NULL, data = NULL,
geom = "smooth", position = "identity",
...,
method = "auto",
formula = y ~ x,
formula = NULL,
se = TRUE,
n = 80,
span = 0.75,
Expand Down Expand Up @@ -86,21 +88,25 @@ StatSmooth <- ggproto("StatSmooth", Stat,

if (max_group < 1000) {
params$method <- "loess"
params$formula <- params$formula %||% (y ~ x)
Copy link
Member

Choose a reason for hiding this comment

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

You're now setting the formula in two places — I don't think you need to set when figuring out which method to use, you can just set it once the method is known.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If the formula isn't set before the message() call just below, then the message could include a NULL instead of the relevant formula, breaking this test for example. Maybe a solution could be to have a logical keep track of whether or not params$method is originally "auto" and then move the message() call down until after the formula has been set once and wrap it in if (using_auto_method), although that doesn't seem great to me.

Copy link
Member

Choose a reason for hiding this comment

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

Oh, I see the problem — now that method and formula are being set independently, you need to break that message up into two pieces. You should only ever inform the user when the parameter was chosen automatically, not when they supplied it.

} else {
params$method <- "gam"
params$formula <- y ~ s(x, bs = "cs")
params$formula <- params$formula %||% (y ~ s(x, bs = "cs"))
}
message("`geom_smooth()` using method = '", params$method,
"' and formula '", deparse(params$formula), "'")
}
if (identical(params$method, "gam")) {
params$method <- mgcv::gam
params$formula <- params$formula %||% (y ~ s(x, bs = "cs"))
} else {
params$formula <- params$formula %||% (y ~ x)
}

params
},

compute_group = function(data, scales, method = "auto", formula = y~x,
compute_group = function(data, scales, method = "auto", formula = NULL,
Copy link
Member

Choose a reason for hiding this comment

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

This should probably have method = "loess" and formula = y ~ x

se = TRUE, n = 80, span = 0.75, fullrange = FALSE,
xseq = NULL, level = 0.95, method.args = list(),
na.rm = FALSE) {
Expand Down
8 changes: 5 additions & 3 deletions man/geom_smooth.Rd
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.