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

Suppress multiple match warning #203

Merged
merged 1 commit into from
Jan 17, 2023

Conversation

DavisVaughan
Copy link
Contributor

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
Copy link
Owner

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?

@gavinsimpson gavinsimpson merged commit 5440cb3 into gavinsimpson:master Jan 17, 2023
@DavisVaughan
Copy link
Contributor Author

Correct!

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.

2 participants