Skip to content

Update the add / remove / update functions to make sure they do not strip the epi_workflow class. #325

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

Closed
rachlobay opened this issue Apr 27, 2024 · 1 comment · Fixed by #328

Comments

@rachlobay
Copy link
Contributor

From a discussion with Daniel: workflows::add_model() is a function, not an S3 generic (like fit()). But we created it in our package as a generic. Then we made methods for it like add_model.epi_workflow(). But within those, we called things like workflows::add_model(). This strips the epi_workflow class from the object.
I’m not sure how we should create our versions. If someone loads workflows after they load epipredict, it will overwrite our generic + methods. This is why we had to move the update.Rmd file into the articles folder (otherwise R CMD check is finding an error based on this where the update_model() function ~ line 170 is stripping the epi_workflow class). We should update the vignette too and can move it back to the vignettes folder after this issue is handled.

The “right” think to do is probably to do a pull request into workflows. But for now, we may have to replace all the add / remove / update functions with add_epi / remove_epi / update_epi versions.

@dajmcdon dajmcdon mentioned this issue Apr 30, 2024
3 tasks
@brookslogan
Copy link
Contributor

brookslogan commented Jul 3, 2024

I just came across ?Methods_for_Nongenerics which provides an alternate route... but requiring some S4 dispatch ("generic" here means S4, they refer to S3 as ' "Generic" ').

  • Pro: wf <- epi_workflow(r) %>% add_model(parsnip::linear_reg()) works even if attaching workflows after epipredict or without attaching epipredict. [epipredict probably still needs to be loaded, but that'd also be true if we were just providing an S3 method for an upstream S3 generic]
  • Same(?): wf <- epi_workflow(r) %>% workflows::add_model(parsnip::linear_reg()) still doesn't work.
  • Con: if attaching epipredict after workflows or without attaching workflows, if you print add_model you get a chunk of text describing it as an S4 generic which can be intimidating.

Here's a rough setup if it's of interest

#' @importFrom methods setOldClass setGeneric setMethod
NULL

setOldClass("epi_workflow")

#' Add a model to an object
#'
#' @param x ....
#' .....
#'
#' @importFrom workflows add_model
#' @export
setGeneric("add_model")

#' @method add_model epi_workflow
#' @export
add_model.epi_workflow <- function(x, spec, ..., formula = NULL) {
  stop("hooray!")
}
# ^ Registering as an S3 method probably doesn't change any behavior currently,
# as we'd never be actually S3-dispatching. However, if workflows does
# eventually introduce an S3 generic, then this would make the workflows package
# start to S3-dispatch to this method, instead of continuing to ignore our
# methods, and we'd remain compatible with previous versions of workflows.


#' You can describe the method for this specific signature separately. That
#' appears to pacify a package check. This help is accessible via
#' `help("add_model,epi_workflow-method")`, though
#' `method?add_model("epi_workflow")` doesn't seem to work properly with just
#' this setup. (Nor does `methods?add_model`.)
#'
#' @export
setMethod("add_model", "epi_workflow", add_model.epi_workflow)

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 a pull request may close this issue.

2 participants