You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
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.
The text was updated successfully, but these errors were encountered:
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 setMethodNULL
setOldClass("epi_workflow")
#' Add a model to an object#'#' @param x ....#' .....#'#' @importFrom workflows add_model#' @exportsetGeneric("add_model")
#' @method add_model epi_workflow#' @exportadd_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`.)#'#' @exportsetMethod("add_model", "epi_workflow", add_model.epi_workflow)
From a discussion with Daniel:
workflows::add_model()
is a function, not an S3 generic (likefit()
). But we created it in our package as a generic. Then we made methods for it likeadd_model.epi_workflow()
. But within those, we called things likeworkflows::add_model()
. This strips theepi_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 theupdate_model()
function ~ line 170 is stripping theepi_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.
The text was updated successfully, but these errors were encountered: