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

Fix generic signature problems identified with new R-devel #1065

Merged
merged 5 commits into from
Mar 9, 2023

Conversation

kylebaron
Copy link
Collaborator

@kylebaron kylebaron commented Mar 8, 2023

Summary

CRAN has come up with some methods where the signature does not match the generic. This has been there for a long time and I guess R-devel changed a bit to detect this; it isn't there in R-release or R-devel up to a couple of days ago.

@kyleam - I'm not worried about the compile() method. But I really want to be able to have an as_tibble() method for the simulation output class. I had the signature right at some point but the generic changed and that caused the problems.

I'm heading toward just getting the methods matched up with the generics and sending that back. That seemed to fix the issue when testing R-devel from yesterday. Wondering about your thoughts on safe / sustainable approach going forward?

Results

Dear maintainer,

Please see the problems shown on
<https://cran.r-project.org/web/checks/check_results_mrgsolve.html>.

Please correct before 2023-03-22 to safely retain your package on CRAN.

Best,
-k

https://cran.r-project.org/web/checks/check_results_mrgsolve.html

Tests

Reproduce check issue on 1.0.8

I've only been able to reproduce on (very) recent R-devel.

R CMD check mrgsolve_1.0.8.tar.gz
* using log directory ‘/Users/kyleb/git/m4solve/mrgsolve.Rcheck’
* using R Under development (unstable) (2023-03-07 r83950)
* using platform: aarch64-apple-darwin20 (64-bit)
* R was compiled by
    Apple clang version 13.0.0 (clang-1300.0.29.3)
    GNU Fortran (GCC) 12.0.1 20220312 (experimental)
* running under: macOS Ventura 13.2
...
* checking dependencies in R code ... OK
* checking S3 generic/method consistency ... WARNING
compiled:
  function(x, ...)
compiled.mrgmod:
  function(x, status)

as_tibble:
  function(x, ..., .rows, .name_repair, rownames)
as_tibble.mrgsims:
  function(.data_, ...)

See section ‘Generic functions and methods’ in the ‘Writing R
Extensions’ manual.
* checking replacement functions ... OK
* checking foreign function calls ... OK

The changes in this PR seem to fix the issue

 R CMD check mrgsolve_1.0.8.9001.tar.gz
* using log directory ‘/Users/kyleb/git/m4solve/mrgsolve.Rcheck’
* using R Under development (unstable) (2023-03-07 r83950)
* using platform: aarch64-apple-darwin20 (64-bit)
* R was compiled by
    Apple clang version 13.0.0 (clang-1300.0.29.3)
    GNU Fortran (GCC) 12.0.1 20220312 (experimental)
* running under: macOS Ventura 13.2
...
* checking dependencies in R code ... OK
* checking S3 generic/method consistency ... OK
*

@kylebaron kylebaron requested a review from kyleam March 8, 2023 18:08
Copy link
Contributor

@kyleam kyleam left a comment

Choose a reason for hiding this comment

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

Hmm, I wonder if this is the result of R's revision 83941. Anyway I can trigger the warnings too with R-devel 2023-03-07 r83950 and mrgsolve 1.0.8. I think the as_tibble change here does more than it needs to.

.name_repair = .name_repair,
rownames = rownames,
...
)
}
Copy link
Contributor

@kyleam kyleam Mar 8, 2023

Choose a reason for hiding this comment

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

Was it just unhappy about the name of the first parameter? I can get the warning to go away on my end with just (x, ...) as the signature.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks for testing that out. I thought methods had to have all the arguments of the generic.

https://rstudio.github.io/r-manuals/r-exts/Generic-functions-and-methods.html

I could have sworn I ran into this before ...

Copy link
Collaborator Author

@kylebaron kylebaron Mar 9, 2023

Choose a reason for hiding this comment

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

@kyleam I went with the simpler fix as you suggested. I don't know what the status is of the as_data_frame() generic ... so I changed that argument too. I don't care if that stays or goes at this point so I just changed it to try to stay out of trouble.

> dplyr::as_data_frame
function(x, ...) {
  deprecate_warn("2.0.0", "as_data_frame()", "as_tibble()",
    details = "The signature and semantics have changed, see `?as_tibble`."
  )

  as_tibble(x, ...)
}

I sent this to winbuilder to make sure it passed there. Will update when that comes back.

Copy link
Contributor

Choose a reason for hiding this comment

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

I thought methods had to have all the arguments of the generic.

https://rstudio.github.io/r-manuals/r-exts/Generic-functions-and-methods.html

My understanding is that, although a method needs to accept all the arguments of the generic, it's okay for a method to fold arguments into .... In the link above, using ... in this scenario is referenced a bit in the predict.lm problem that's mentioned: "as predict.lm had neither a dispersion nor a ... argument, NextMethod could no longer be used".

Note that some of tibble's own methods use (x, ...):

tibble $ git describe HEAD
v3.2.0.9000

tibble $ git grep '^as_tibble\.' 'R/*.R'
R/as_tibble.R:as_tibble.data.frame <- function(x, validate = NULL, ...,
R/as_tibble.R:as_tibble.list <- function(x, validate = NULL, ..., .rows = NULL,
R/as_tibble.R:as_tibble.matrix <- function(x, ..., validate = NULL, .name_repair = NULL) {
R/as_tibble.R:as_tibble.poly <- function(x, ...) {
R/as_tibble.R:as_tibble.ts <- function(x, ..., .name_repair = "minimal") {
R/as_tibble.R:as_tibble.table <- function(x, `_n` = "n", ..., n = `_n`, .name_repair = "check_unique") {
R/as_tibble.R:as_tibble.NULL <- function(x, ...) {
R/as_tibble.R:as_tibble.default <- function(x, ...) {

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Great. This really simplifies things. I'll merge this and then send it to CRAN; the winbuilder (failed on 1.0.8) came back fine now.

@kylebaron kylebaron requested a review from kyleam March 9, 2023 04:18
@kylebaron kylebaron merged commit 5b20384 into main Mar 9, 2023
@kylebaron kylebaron deleted the fix/generics branch March 9, 2023 14:26
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.

None yet

2 participants