gavinsimpson / gratia

ggplot-based graphics and useful functions for GAMs fitted using the mgcv package
https://gavinsimpson.github.io/gratia/
Other
206 stars 28 forks source link

Suppress multiple match warning #203

Closed DavisVaughan closed 1 year ago

DavisVaughan commented 1 year ago

This PR makes your package compatible with the next version of dplyr:

The join functions in dplyr (like left_join()) now return a warning by default when a row in x matches multiple rows in y. While this is typical SQL behavior, it is often unexpected during data analysis (many people don't even know it is possible), so we've decided to make this a warning. In dplyr 1.1.0, you silence this warning with multiple = "all". In the meantime, to be compatible with both dev and CRAN dplyr we need to work around broken tests of yours that were expecting no output.

I think I've fixed this by suppressing the warning anywhere you called a join function. I'll admit I was having trouble running your tests (I am on a Mac), but the changes I made seem to align with the failures I saw in the cloud revdepcheck results:

══ Failed tests ════════════════════════════════════════════════════════════════
── Failure ('test-draw-gam.R:69'): draw.gam works for dlnm_m ───────────────────
`plt <- draw(dlnm_m, rug = FALSE)` produced warnings.
── Failure ('test-draw-smooth-estimates.R:56'): draw.smooth_estimates works for su_m_bivar ──
`plt <- draw(smooth_estimates(su_m_bivar, dist = 0.1))` produced warnings.
── Failure ('test-draw-smooth-estimates.R:62'): draw.smooth_estimates works for su_m_bivar_te ──
`plt <- draw(smooth_estimates(su_m_bivar_te, dist = 0.1))` produced warnings.
── Failure ('test-draw-smooth-estimates.R:67'): draw.smooth_estimates works for su_m_bivar_t2 ──
`plt <- draw(smooth_estimates(su_m_bivar_t2, dist = 0.1))` produced warnings.
── Failure ('test-smooth-estimates.R:103'): smooth_estimates works with a bivariate TPRS smooth with dist ──
`sm <- smooth_estimates(su_m_bivar, "s(x,z)", n = 50, dist = 0.1)` produced warnings.
── Failure ('test-smooth-estimates.R:122'): smooth_estimates works with a bivariate te smooth with dist ──
`sm <- smooth_estimates(su_m_bivar_te, "te(x,z)", n = 50, dist = 0.1)` produced warnings.

[ FAIL 6 | WARN 50 | SKIP 156 | PASS 1686 ]

If you could check against dev dplyr yourself to confirm, that would be great!

We plan to submit dplyr 1.1.0 on January 27th.

This should be compatible with both dev and CRAN dplyr. It would help us out if you could go ahead and send a patch version of your package to CRAN ahead of time! Thanks!

gavinsimpson commented 1 year ago

Thanks for the heads up @DavisVaughan I'll try to get something to CRAN before the 27th. The current behaviour was anticipated (required). I presume that after dyplr 1.1.0 is accepted to CRAN, it would be preferred to require this version and use multiple = "all" explicitly in these left_join() calls where this behaviour is expected?

DavisVaughan commented 1 year ago

Correct!