cmu-delphi / epipredict

Tools for building predictive models in epidemiology.
https://cmu-delphi.github.io/epipredict/
Other
8 stars 8 forks source link

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

Closed rachlobay closed 2 months ago

rachlobay commented 2 months ago

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.

brookslogan commented 3 days ago

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" ').

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)