Stat smooth+reml #3402
Stat smooth+reml #3402
Conversation
Co-Authored-By: Hadley Wickham <[email protected]>
|
Can you please find @gaborcsardi and get him to help you tweak your ESS settings so that it doesn't reformat these files? |
@ikosmidis I am at the table next to Hadley if you need help. You probably need For the spaces/indentation, and to avoid the fancy ess comment character. |
R/stat-smooth.r
Outdated
| if (identical(method, "loess")) { | ||
| method.args$span <- span | ||
| } | ||
|
|
||
| if (is.character(method)) method <- match.fun(method) | ||
| ## If gam and gam's method is not specified by the user then use REML | ||
| if (identical(method, "gam") && is.null(method.args$method)) { |
There was a problem hiding this comment.
This check should probably be identical(method, mgcv::gam) and happen after the conversion from character to function in the next block.
There was a problem hiding this comment.
I changed the design slightly here. In master params$method was replaced by mgcv::gam if it was equal to "gam", within setup_params. Now params$method keeps being "gam" and matching names to functions happens in compute_group. This is to preserve symmetry with how the other methods are handled (e.g. loess) where method is match.funed in compute_group.
The is.character check may as well be redundant but I kept it in just in case I was breaking anything I could not foresee.
Happy to revert to the earlier design and test functions being identical instead of character strings, if this causes any issues; though ggplot2 tests were successful my side.
There was a problem hiding this comment.
Or perhaps an alternative closer to what you are suggesting, that avoids checking twice if method is "gam" can be something like:
if (is.character(method)) {
if (identical(method, "gam")) {
method <- mgcv::gam
if (is.null(method.args$method)) {
method.args$method <- "REML"
}
} else {
method <- match.fun(method)
}
}Again the is.character may not be needed anymore.
There was a problem hiding this comment.
OK; just figured out that ggplot2 wants the user to be able to supply functions for method as well as character strings so checking method against mgcv::gam after matching makes more sense. Just pushed a fix.
|
Thanks @ikosmidis! |
|
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/ |
Closes issue #2630