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

Simplify API or add narrative explanation #71

Closed
richelbilderbeek opened this issue Jul 21, 2018 · 4 comments
Closed

Simplify API or add narrative explanation #71

richelbilderbeek opened this issue Jul 21, 2018 · 4 comments

Comments

@richelbilderbeek
Copy link
Member

Feedback dwinter:

Both the user-interface and the code for specifying distributions is very complex, and I am concerned about how easily this package will be to maintain. @bjoelle has made some suggestions about a more modular approach to the internal code and possibly grouping like-functions into files. I think this would be a good idea, I also wonder if you could also consider a more high-level approach to specifying these models in user-exposed code. For instance, how much power/flexibility would be lost by losing the create_*_param() functions and instead doing something like this:

node_calibration <- create_gamma_distr(alpha=1, beta=10, estimate=FALSE)

I appreciate there may be reasons for taking the approach you have, but the current does make for a complex API. Whatever the API, some narrative examples
of setting up these paramaters in the documentation would be helpful.

@richelbilderbeek
Copy link
Member Author

This Issue is the reason for

@richelbilderbeek
Copy link
Member Author

I agree with dwinter to simplify the interface. His suggestion is:

create_gamma_distr(alpha = 1, beta = 10, estimate = FALSE)

Taking a look at BEAUti shows this is an oversimplifictation, because both/either/neither of alpha and beta can be estimated:

create_gamma_distr

An alternative interface to arrive at would then be:

create_gamma_distr(alpha = 1, beta = 10, estimate_alpha = FALSE, estimate_beta = FALSE)

But this is not a great improvement over what is implemented currently:

create_gamma_distr(
  alpha = create_alpha_param(value = 1, estimate = FALSE),
  beta = create_beta_param(value = 10, estimate = FALSE)
)

@richelbilderbeek
Copy link
Member Author

Perhaps a different improvement is to rename alpha to alpha_param (and for other parameters as well). From that appoach, a user can conclude what to do...

@richelbilderbeek
Copy link
Member Author

Thanks to #45 ('Hyper parameters are not estimated') must parameters can be upped to a structure from only a value. The additional attributes are:

  • ID: should be NA
  • estimate: will be FALSE (thus no hyper parameter)

This will simplify the interface a lot.

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

1 participant