Skip to content

This issue was moved to a discussion.

You can continue the conversation there. Go to discussion →

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

Interface unification with epinow2() wrapper function? #191

Closed
hsbadr opened this issue Nov 30, 2020 · 3 comments
Closed

Interface unification with epinow2() wrapper function? #191

hsbadr opened this issue Nov 30, 2020 · 3 comments
Labels
enhancement New feature or request question Further information is requested

Comments

@hsbadr
Copy link
Contributor

hsbadr commented Nov 30, 2020

Both epinow() and epinow_regional() take common arguments, except for the region column in the reported_cases and the two extra epinow_regional()'s arguments (non_zero_points and summary_args) that can be ignored by epinow(). So, it's probably better to define a wrapper function for both and decide which one to call based on reported_cases$region. Here's an initial version I'm currently using:

epinow2 <- function(reported_cases, ...) {
  if (is.null(reported_cases$region)) {
    local_epinow2 <- function(reported_cases, ..., non_zero_points, summary_args) {
      epinow(reported_cases, ...)
    }
    local_epinow2(reported_cases, ...)
  } else {
    regional_epinow(reported_cases, ...)
  }
}

Note that local_epinow2 can be dropped (i.e., calling epinow() directly) if epinow accepts non_zero_points and summary_args, regardless if it'll ignore or use them.

Ideally, all documents would refer to the new epinow2() function while exporting the current functions for compatibility.

@seabbs
Copy link
Contributor

seabbs commented Dec 2, 2020

I'm not totally convinced this would be that user friendly an idea? regional_epinow also accepts lists for all _opts args so that you can specify region specific settings which might get very confusing if everything was run from a single wrapper. Happy to canvas opinion on this though.

@hsbadr
Copy link
Contributor Author

hsbadr commented Dec 2, 2020

Yes, I know that, The user can add a region column to the reported_cases and may provide region-specific options (e.g., via opts_list). The logic here is similar to providing a formula for single- vs multi-variate analysis (or single vs multi-regional epi estimates here). I think that the wrapper function will be a good addition as it won't affect the current user interface: both epinow() and regional_epinow() will be there and separate. The proposed epinow2() is just a wrapper to call the appropriate function.

@seabbs seabbs added enhancement New feature or request question Further information is requested labels Jan 17, 2023
@sbfnk sbfnk mentioned this issue Jul 18, 2023
18 tasks
@sbfnk
Copy link
Contributor

sbfnk commented Jan 29, 2024

Converting to discussion in case anyone else would like to chime in - for now I'm considering this not a priority for implementaiotn.

@epiforecasts epiforecasts locked and limited conversation to collaborators Jan 29, 2024
@sbfnk sbfnk converted this issue into discussion #535 Jan 29, 2024

This issue was moved to a discussion.

You can continue the conversation there. Go to discussion →

Labels
enhancement New feature or request question Further information is requested
Projects
None yet
Development

No branches or pull requests

3 participants