Full package review for v0.1.0 release

[joshwlambert]

This PR is for the full package review of {epiparameter}.

After this review is complete I will:

• complete the release checklist
• create a Github release
• submit {epiparameter} to CRAN

[joshwlambert]

@Bisaloo thanks for the comments.

In particular the need to reduce the package API/namespace. I think this would be a good direction to go for subsequent releases, and in general I think the code base can be simplified in a number of areas.

I like the idea of:

epiparam() |> 
  mutate(params = convert_summary_stats(prob_distribution, mean, sd))

But given that there is a vignette for the current conversion functions, I would need to rewrite the vignette for this change. Therefore I propose this to be a good task for v0.2.0.

[Bisaloo]

I would strongly encourage you to do this before the first CRAN release as breaking changes are very annoying to deal with once the package is on CRAN. You'd have to either use a safe but complex deprecation process or pull the plug without safeguards and potentially cause a lot of issues for the early users, who as a result may not return to this package.

[joshwlambert]

bind_epiparam()

Suggest updating Details to tweak the definition of the epiparam class here, as I understand that epiparam() returns the package library, but any epi- objects can be bound together to form an epiparam class

The documentation states that any epi object can be bound to an existing <epiparam> object. The function does not bind any two epi objects.

Suggest use is.list() rather than inherits(x, "list"), unless there is a reason to not?

is.list() and inherits(x, "list") are not functionally equivalent. Difference is due to it being atomic/recursive or inheriting from a list.

> is.list(data.frame())
[1] TRUE
> inherits(data.frame(), "list")
[1] FALSE

Suggest updating error message at the end of if-else ladder to add data.frame as a permitted input

Done

PR #171.

[joshwlambert]

cbind_epiparam()

I'm not sure whether this function is necessary. I don't see it used elsewhere in the package or showcased in the vignettes. It's already possible to assign columns to an epiparam object as to a dataframe.

Would it make sense to just rely on dplyr::add_column() for this? I see that epiparameter is exporting a very long list of functions and I'm worried it'll be very difficult / time-consuming to maintain in the long run.

I have removed the cbind_epiparam() function and added a line to the getting started vignette about the use of dplyr::bind_col() and tibble::add_column().

PR #171.

[joshwlambert]

In particular, it has a very large number of functions and any action that would reduce the number of functions (including internal functions but especially exported functions) would improve readability & maintainability.
Another sign of this is the presence of copied/pasted code chunks, especially when branching to deal with different distributions. Ideally, supporting a new probability should only require to add the low-level parameter conversion functions. A rule of thumb could be that adding a new probability distribution should not create any new exported function.
A solution could be to have a single function for all distribution and provide the distribution as an argument. This has very nice side-effects, like the fact you can automatically dispatch to the correct function without switch() or if/else.

The number of conversion functions was reduced to two in PR #165

[joshwlambert]

Could the citation field in epidist objects be stored in a strict structure (e.g., bibentry()) rather than free text to avoid complicated and error-prone regexes here and in create_epidist_citation()?

Citations are handled as <bibentry> since PR #168.

[joshwlambert]

calc_dist_params()

1. Suggest converting the Details to a numbered list.
2. I'm not very clear on what the prob_dist_params argument is for - there's some input checking on it, but this argument is never operated upon - only returned after assignment at the end. Might be better to initialise it as NA in that case and take away the fn argument?

These changes as well as changes to other <epidist> utility functions have been made in PR #175

[joshwlambert]

Closing this PR without merging. Initially I had started pushing to the review branch but subsequently decided it would be better to tackle related issues in separate, smaller pull requests (which include the changes initially made in review).

Thanks to all reviewers for taking the time to look through the code and make comments and suggestions.

The PR containing the changes are linked above in this PR.

I will now work to finalise the package ready for an official v0.1.0 release.