Skip to content
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

function arguments #65

Open
sbfnk opened this issue Nov 28, 2023 · 8 comments
Open

function arguments #65

sbfnk opened this issue Nov 28, 2023 · 8 comments

Comments

@sbfnk
Copy link
Contributor

sbfnk commented Nov 28, 2023

At the moment model parameters sit side-by-side as function arguments with simulation control arguments (i.e. how many simulations, when to stop etc.). If functionality is added to the model in the future, this will lead to an ever-expanding number of arguments, at the expense of usability. It might be good to think about an alternative interface where, e.g. model parameters are passed through function calls or as lists or via some other mechanism.

@adamkucharski
Copy link

Wonder if there's any functionality from {epidemics} that might be useful here in wrapping up parameters to pass to model (maybe just for alignment in styles, rather than taking on as dependency)?

@pearsonca
Copy link
Collaborator

pearsonca commented Nov 29, 2023

for consistency with general R language conventions, should probably be:

res <- scenario_sim(
  n = 10, # n is the stand name for number of samples
  parms = ringbp::parameters(), # the deSolve mispelling is widely used for inexplicable reasons 
  control = ringbp::control_opts() # gives defaults when empty
)

... then all the documentation for pars, control options is migrated to / wrapped up in the parameters() and control_opts() documentation.

@pearsonca
Copy link
Collaborator

also, could embrace the abbr_function() convention, and this would look like:

res <- rbp_simulate(n = 10, parms = rbp_parameters(), control = rbp_control())

@adamkucharski
Copy link

That makes sense. Regardless of design choice, would also be useful to have ability to pass distributions like incubation period and estimates of k etc. from {epiparameter} (and {epireview} once we finalise planned interoperability) into both {epichains} and {ringbp}.

@pearsonca
Copy link
Collaborator

pearsonca commented Nov 29, 2023

I imagine that rbp_parameters() (or whatever) would roughly be:

rbp_parameters <- function() {
  rprocessX = ...,
  rprocessY = ...,
  etc
}

... where rprocessX is either a function(n) ... that can just be drawn from OR a standard collection object of distribution elements (which are then used to construct such a function internally).

Assuming that's what those other packages yield, then sure.

(also, the r prefix is a deliberate choice - again, lean into the R sampling conventions).

@sbfnk
Copy link
Contributor Author

sbfnk commented Nov 29, 2023

I'm not a fan of the rbp_... syntax. R itself provides a mechanism for that (as you wrote yourself in e.g. ringbp::parameters() - which I'm inclined to agree is the way forward).

@pearsonca
Copy link
Collaborator

pearsonca commented Nov 29, 2023

I'm not a fan of the rbp_... syntax. R itself provides a mechanism for that (as you wrote yourself in e.g. ringbp::parameters() - which I'm inclined to agree is the way forward).

shrug; I have mixed feelings.

In the case of ringbp, I think the package name itself is sufficient to get my prioritized benefit (tab completion to find functions). (some packages have annoyingly long names, but 6 chars is trim enough imo).

I don't like the longer / clunkier prefix_names, but they do largely deal with namespace collisions, which is other main argument for them (and is not an insignificant one for less savvy coders, but not generally a problem for me).

@pearsonca
Copy link
Collaborator

n.b. what I've proposed above would likely close #29 as well (assuming sensible naming conventions).

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

No branches or pull requests

3 participants