CDCgov / ww-inference-model

An in-development R package and a Bayesian hierarchical model jointly fitting multiple "local" wastewater data streams and "global" case count data to produce nowcasts and forecasts of both observations
https://cdcgov.github.io/ww-inference-model/
Apache License 2.0
17 stars 2 forks source link

49 output class creation #59

Closed kaitejohnson closed 3 months ago

kaitejohnson commented 3 months ago

This is a draft PR building off of @gvegayon's to create a wwinference_fit (or maybe we should make the name shorter) class object that then is passed to overloaded downstream functions. I am going to rewrite this to align with what was outlined in #49, and then modify the vignette and add testing accordingly.

kaitejohnson commented 3 months ago

@gvegayon OK I think I am sort of stuck here on how I should do the documentation!

Should the ww_output object in get_draws_df.wwinference_fit() have documentation right above it, or is it an additional thing to document in the main get_draws_df() function.

Does this part #' @rdname get_draws_df just mean that the .default() and .wwinference_fit() versions of the function should point to the get_draws_df() documentation?

gvegayon commented 3 months ago

Should the ww_output object in get_draws_df.wwinference_fit() have documentation right above it, or is it an additional thing to document in the main get_draws_df() function.

Does this part #' @rdname get_draws_df just mean that the .default() and .wwinference_fit() versions of the function should point to the get_draws_df() documentation? -- @kaitejohnson

The rdname roxygen tag indicates the function is being documented elsewhere. This means all the functions sharing rdname will share the *.man file. Moreover, functions added via rdname create aliases for the manual, so typing ?get_draws_df works the same as ?get_draws_df.default.

Now, where to document parameters via @param? The answer is: anywhere. As long as you do it once. roxygen::roxygenise will collect all the @param tags (all the doc tags) and put them in the corresponding manual. But, if you add the same entry multiple times, it will create duplicates.' Here is a general pattern you can consider:

Here is an example (ref)

#' @export
#' @rdname virus
#' @param model An object of class `epiworld_model`.
#' @param virus An object of class `epiworld_virus`
#' @param proportion Deprecated. 
#' @returns 
#' - The `add_virus` function does not return a value, instead it adds the 
#' virus of choice to the model object of class [epiworld_model].
add_virus <- function(model, virus, proportion) {
kaitejohnson commented 3 months ago

Ok dumb question but is the .default() one the main function? If so, we should have the majority of the documentation there, and then just add the @param for the wwinference_fit class object in the .wwinference_fit() sub function?

kaitejohnson commented 3 months ago

Putting this here for reference https://cran.r-project.org/doc/manuals/R-exts.html#Generic-functions-and-methods as I am trying to understand how the "methods" here are working and where documentation of arguments should go

kaitejohnson commented 3 months ago

A little perplexed on why the one where I pass in the args explictly doesn't seem to be working in the vignette (it works when I put those functions into my global environment)

kaitejohnson commented 3 months ago

@gvegayon Will wait for #54 to be merged then ask for your review on this!

kaitejohnson commented 3 months ago

Then just a few more steps to add some testing to the diagnostic and draw extraction functions

kaitejohnson commented 3 months ago

A little perplexed on why the one where I pass in the args explictly doesn't seem to be working in the vignette (it works when I put those functions into my global environment)

I realized I need to export everything

gvegayon commented 3 months ago

It seems there's a scoping issue here. I see the params object defined at an upper level, so I am not sure what's going on.

kaitejohnson commented 3 months ago

@gvegayon Pending CI passes, I think this is ready for you to review for a merge into your branch, and then add to with some of the other overloaded functions you'd recommend (like summary and others you mentioned). I will add some tests of the get_draws_df() function and plotting functions, but I think those can be separate PRs. More curious to hear if things look ok with how I overloaded the functions and did the documentation.