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.

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

stat_smooth() drops failed groups #5371

Merged
merged 7 commits into from
Oct 2, 2023
Merged

Conversation

teunbrand
Copy link
Collaborator

This PR aims to fix #5352.

Briefly, it wraps the fitting/predicting procedure in a tryCatch() block to rethrow errors as warnings and drops the group for which the procedure fails. This prevents one bad group stopping the fitting in all other groups.

The example from #5352 with this PR (in particular, see the very last warning):

devtools::load_all("~/packages/ggplot2")
#> ℹ Loading ggplot2
#> Warning: package 'testthat' was built under R version 4.3.1

diamonds |> 
  subset(cut == "Ideal" & color == "J") |>
  ggplot( aes(carat, price)) +
  geom_point(aes(color = clarity)) +
  geom_smooth(aes(color = clarity))
#> `geom_smooth()` using method = 'loess' and formula = 'y ~ x'
#> Warning in simpleLoess(y, x, w, span, degree = degree, parametric = parametric,
#> : span too small.  fewer data values than degrees of freedom.
#> Warning in simpleLoess(y, x, w, span, degree = degree, parametric = parametric,
#> : at 0.9598
#> Warning in simpleLoess(y, x, w, span, degree = degree, parametric = parametric,
#> : radius 0.00010404
#> Warning in simpleLoess(y, x, w, span, degree = degree, parametric = parametric,
#> : all data on boundary of neighborhood. make span bigger
#> Warning in simpleLoess(y, x, w, span, degree = degree, parametric = parametric,
#> : pseudoinverse used at 0.9598
#> Warning in simpleLoess(y, x, w, span, degree = degree, parametric = parametric,
#> : neighborhood radius 0.0102
#> Warning in simpleLoess(y, x, w, span, degree = degree, parametric = parametric,
#> : reciprocal condition number 1
#> Warning in simpleLoess(y, x, w, span, degree = degree, parametric = parametric,
#> : at 3.0202
#> Warning in simpleLoess(y, x, w, span, degree = degree, parametric = parametric,
#> : radius 0.00010404
#> Warning in simpleLoess(y, x, w, span, degree = degree, parametric = parametric,
#> : all data on boundary of neighborhood. make span bigger
#> Warning in simpleLoess(y, x, w, span, degree = degree, parametric = parametric,
#> : There are other near singularities as well. 0.00010404
#> Warning in simpleLoess(y, x, w, span, degree = degree, parametric = parametric,
#> : zero-width neighborhood. make span bigger

#> Warning in simpleLoess(y, x, w, span, degree = degree, parametric = parametric,
#> : zero-width neighborhood. make span bigger
#> Warning: Failed to fit group 1.
#> Caused by error in `predLoess()`:
#> ! NA/NaN/Inf in foreign function call (arg 5)

Created on 2023-07-27 with reprex v2.0.2

@teunbrand
Copy link
Collaborator Author

As an aside, is there some smart way to reduce repeated parts in the warnings? In particular, the call getting mentioned several times is a bit annoying and it would seem cleaner to just warn like so:

#> Warning in simpleLoess(y, x, w, span, degree = degree, parametric = parametric,
#> : span too small.  fewer data values than degrees of freedom.
#> : at 0.9598
#> : radius 0.00010404
#> : all data on boundary of neighborhood. make span bigger
# etc...
#> Warning: Failed to fit group 1.
#> Caused by error in `predLoess()`:
#> ! NA/NaN/Inf in foreign function call (arg 5)

Copy link
Member

@thomasp85 thomasp85 left a comment

Choose a reason for hiding this comment

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

I'm not sure what we can do about the excessive warnings. Summarising them would require us to make some very hard expectations about the warnings coming from the call...

R/stat-smooth.R Outdated Show resolved Hide resolved
Copy link
Member

@thomasp85 thomasp85 left a comment

Choose a reason for hiding this comment

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

LGTM

@teunbrand teunbrand merged commit 88d0517 into tidyverse:main Oct 2, 2023
11 of 12 checks passed
@teunbrand teunbrand deleted the smooth_groups branch October 2, 2023 12:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

geom_smooth() fails for every group if only one group is wrong
2 participants