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

create gt_opts as an alias for generation_time_opts #564

Closed
sbfnk opened this issue Feb 21, 2024 · 4 comments · Fixed by #698
Closed

create gt_opts as an alias for generation_time_opts #564

sbfnk opened this issue Feb 21, 2024 · 4 comments · Fixed by #698
Labels
question Further information is requested

Comments

@sbfnk
Copy link
Contributor

sbfnk commented Feb 21, 2024

As we move towards extending the _opts interface to specifying the data and preprocessing (#346) we are adding complexity to the user interaction. Where in the past a user would have written something like:

epinow(reported_cases = cases, generation_time = list(mean = 3, sd = 1))

we're moving towards requiring

epinow(
  data = data_opts(cases),
  generation_time = generation_time_opts(Gamma(mean = 3, sd = 1)))
)

I'm wondering if we should provide a simpler version where if any of the arguments requiring an _opts helper function are not specified using said helper functions we should call it for the user and pass on the first argument. This would enable the user to specify some of their desired (e.g. case data, generation time) without being exposed to the full interface.

This would mean that one could write

epinow(data = cases, generation_time = Gamma(mean = 3, sd = 1))

with the same results as the calls above.

@sbfnk sbfnk added the question Further information is requested label Feb 21, 2024
@jamesmbaazam
Copy link
Contributor

I believe we moved towards enforcing the use of the *_opts() interface because of situations where wrong options are passed to an argument like pointed out here #337 (comment). If we are to go by the suggestion here, I wonder if we are not putting that burden on ourselves to ensure that the right arguments are passed. It could also lead to unexpected bugs in nuanced cases like the generation time vs delays argument options.

I think the current interface may be verbose but it forces the user to make intentional choices and makes its consequence easier to debug as well.

@seabbs
Copy link
Contributor

seabbs commented Feb 26, 2024

I'm wondering if we should provide a simpler version where if any of the arguments requiring a

Could we provide some short aliases for the annoyingly long ones

@sbfnk
Copy link
Contributor Author

sbfnk commented Feb 27, 2024

I'm wondering if we should provide a simpler version where if any of the arguments requiring a

Could we provide some short aliases for the annoyingly long ones

yes could have gt_opts() map to generation_time_opts() as the main culprit. Or perhaps even rename it? Would then also have to rename the argument if wanting to keep the consistent arg = arg_opts() structure.

Just to try it out, it would make:

epinow(
  data = data_opts(cases),
  gt = gt_opts(Gamma(mean = 3, sd = 1)))
)

Clarity vs brevity, what do people think?

@seabbs
Copy link
Contributor

seabbs commented Feb 28, 2024

Personally I like the idea of an alias here but keep the descriptive arg name and the long form for the docs?

@sbfnk sbfnk changed the title facilitate access to _opts interface create gt_opts as an alias for generation_time_opts Mar 6, 2024
@sbfnk sbfnk self-assigned this May 1, 2024
@sbfnk sbfnk added this to the CRAN v1.6 release milestone May 1, 2024
@sbfnk sbfnk removed their assignment May 1, 2024
@sbfnk sbfnk closed this as completed in #698 Jul 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Further information is requested
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants