Full package review for v0.3.0 release

[joshwlambert]

This PR is to provide a platform to review the entirety of the package.

Once this review concludes I will release v0.3.0 on GitHub and submit to CRAN. This release will occur after {epiparameter} is accepted by CRAN.

Please see the NEWS.md file for an overview of changes between v0.2.0 and v0.3.0. If you would prefer to review with a partial package review only showing the changes between v0.2.0 and v0.3.0 please let me know and I can open one.

This PR is unconventional as it is not intended for merging or for additional commits (unless minor) and instead comments will be converted to issues and these will be addressed in their own PRs.

[github-actions]

This pull request:

• Adds 46 new dependencies (direct and indirect)
• Adds 9 new system dependencies
• Removes 7 existing dependencies (direct and indirect)
• Removes 2 existing system dependencies

Reach out on slack (#code-review or #help channels) to double check if there are base R alternatives to the new dependencies.

(Note that results may be inaccurate if you branched from an outdated version of the target branch.)

[chartgerink]

I got a few errors while running a check on my local machine:

✖ utils.R:277: @description refers to unavailable topic
  bpmodels::chain_sim.
Caused by error in `find.package()`:
! there is no package called ‘bpmodels’

Given that bpmodels is archived this may be worth deleting or not providing the package link.

There's also the problem that the R CMD CHECK is not completing at this moment, but it seems to be only a wordlist issue:

> if (requireNamespace("spelling", quietly = TRUE)) {
+   spelling::spell_check_test(
+     vignettes = TRUE,
+     error = TRUE,
+     skip_on_cran = TRUE
+   )
+ }
Error: Error: Potential spelling errors: KaTeX, math

Beyond that, it looks good – the scope seems well defined and there's no riff raff beyond it. I left some comments for your consideration, and my only primary question is around evaluating the testing to see whether there is additional situations where testing for failure makes sense, as most tests are looking to observe whether things succeed as expected. That, however is not a blocker from my end. So I say, good work and happy to approve, given that I am confident those small issues will be addressed 😊

[chartgerink]

These are positive tests, and there are no negative tests that ensure things fail as expected. Might it be worth adding this in or does the assertions provide sufficient confidence?

[joshwlambert]

It would be good to discuss testing strategies as this came up in both the {epiparameter} review and this review. Once we've discussed an optimal testing approach I'm happy to look through the tests in this package and update to match the chosen approach.

Specifically in relational to this function, earlier in the project I would write units tests that checked that a function would fail as expected from input checking. However, when using {checkmate} for input checking, as is the case here, I later felt that I was just testing {checkmate} functionality so decided to only tests function errors if I had implemented them myself.

Will leave the tests for this function as is for this release.

[chartgerink]

Somehow these make me think of epiparameter's suite of distribution functions. Would it make sense to have those be part of that package in the future?

[joshwlambert]

This is an interesting point and one I haven't thought of before.

To me these fall into the scope of {superspreading} rather than {epiparameter} as their purpose is to facilitate fitting, which is not part of the scope of {epiparameter} (technically there is a small about of fitting in epiparameter::extract_param() but there is no plan to expand to fitting other distribution types).

There is an open discussion on where probability distribution functions should live: https://github.com/orgs/epiverse-trace/discussions/162. We can discuss further on the discussion board 😃.

[joshwlambert]

Thanks @chartgerink for the comments. Several good improvements to the package resulting from this review.

 ✖ utils.R:277: @description refers to unavailable topic
   bpmodels::chain_sim.
Caused by error in `find.package()`:
 ! there is no package called ‘bpmodels’

Given that bpmodels is archived this may be worth deleting or not providing the package link.

Resolved in 0c57b0e by unlinking the documentation.

There's also the problem that the R CMD CHECK is not completing at this moment, but it seems to be only a wordlist issue:

> if (requireNamespace("spelling", quietly = TRUE)) {
+   spelling::spell_check_test(
+     vignettes = TRUE,
+     error = TRUE,
+     skip_on_cran = TRUE
+   )
+ }
Error: Error: Potential spelling errors: KaTeX, math

Resolved in a9d5f67.

All changes have been made in PR #111. I've linked the specific commits that resolve comments in my responses.

Let's follow-up on testing strategies soon and hopefully it will provide guidance the testing approaches to all Epiverse packages.