Multiple issues: make offspring_dist accept a function

[sbfnk]

• Please check if the PR fulfills these requirements

• I have read the CONTRIBUTING guidelines
• A new item has been added to NEWS.md
• Tests for the changes have been added (for bug fixes / features)
• Docs have been added / updated (for bug fixes / features)

• What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)

This PR changes the offspring_dist argument of likelihood and simulate_chains to expect a function instead of a character string.

• Does this PR introduce a breaking change? (What changes might users need to make in their application due to this PR?)

Yes. No version has been released yet so not going through a deprecation cycle.

• Other information:

Doing this necessitated two further changes:

• remove the construct_offspring_ll_name function and instead integrate the code into likelihood; because of the way the function name is extracted from the offspring_dist argument (using substitute) this can't be passed on to lower level functions; as this function was only used in this one instance I think we can live with the loss
• add an rgborel function; this is a difference from previous behaviour, where the ll function name could be informed from the string passed and now it needs a function with that name; the function itself is not actually called when estimating the likelihood if the corresponding ll function exists so this could, in principle, be an empty dummy function; however, for documentation/clarity purposes it is probably a good idea to have this function anyway

Closes #25
Closes #167

[codecov-commenter]

Codecov Report

Attention: 3 lines in your changes are missing coverage. Please review.

Comparison is base (d3e8d85) 99.45% compared to head (6bf6e90) 98.90%.

❗ Current head 6bf6e90 differs from pull request most recent head e2df869. Consider uploading reports for the commit e2df869 to get more accurate results

Files	Patch %	Lines
R/borel.r	78.57%	3 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #188      +/-   ##
==========================================
- Coverage   99.45%   98.90%   -0.55%     
==========================================
  Files           8        8              
  Lines         546      548       +2     
==========================================
- Hits          543      542       -1     
- Misses          3        6       +3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[sbfnk]

Might need updating in line with #190

[jamesmbaazam]

LGTM. Thanks for the change. I believe it resolves #50 as well.

[jamesmbaazam]

Might need updating in line with #190

Oh, I just merged that. You can rebase on main.

[sbfnk]

LGTM. Thanks for the change. I believe it resolves #50 as well.

3 in one go!

[jamesmbaazam]

Please feel free to rebase and merge.

[jamesmbaazam]

@sbfnk Do you mind me merging this?

[jamesmbaazam]

The issue number in the title undersells the change as it resolves 3 related issues, so it might be better to either add the others or remove the current one.

[sbfnk]

@sbfnk Do you mind me merging this?

I think @Bisaloo was still going to kindly review at some point in the near future.

[Bisaloo]

Thanks, it looks like a good change. I have left a couple of minor comments inline.

[sbfnk]

Not immediately clear to me why the R CMD CHECK action fails as it seems to run the previous version of code in the examples.

[Bisaloo]

The branch needs to be rebased and the following examples updated:

epichains/R/epichains.R

Lines 433 to 443 in d3e8d85

	#' @examples
	#' set.seed(32)
	#' chains_pois_offspring <- simulate_chains(
	#' index_cases = 10,
	#' statistic = "size",
	#' offspring_dist = "pois",
	#' stat_max = 10,
	#' generation_time = function(n) rep(3, n),
	#' lambda = 2
	#' )
	#' head(chains_pois_offspring)

epichains/R/epichains.R

Lines 455 to 464 in d3e8d85

	#' set.seed(32)
	#' chains_pois_offspring <- simulate_chains(
	#' index_cases = 10,
	#' statistic = "size",
	#' offspring_dist = "pois",
	#' stat_max = 10,
	#' generation_time = function(n) rep(3, n),
	#' lambda = 2
	#' )
	#' tail(chains_pois_offspring)

epichains/R/epichains.R

Lines 487 to 505 in d3e8d85

	#' @examples
	#' set.seed(32)
	#' chains <- simulate_chains(
	#' index_cases = 10,
	#' statistic = "size",
	#' offspring_dist = "pois",
	#' stat_max = 10,
	#' generation_time = function(n) rep(3, n),
	#' lambda = 2
	#' )
	#' chains
	#'
	#' # Aggregate cases per time
	#' cases_per_time <- aggregate(chains, by = "time")
	#' head(cases_per_time)
	#'
	#' # Aggregate cases per generation
	#' cases_per_gen <- aggregate(chains, by = "generation")
	#' head(cases_per_gen)

[sbfnk]

The branch needs to be rebased and the following examples updated:

Thanks but these are no longer present in 9953900 which failed the checks. I'm confused.

[sbfnk]

Does the action not check out a force pushed version?

[Bisaloo]

Thanks but these are no longer present in 9953900 which failed the checks. I'm confused.

They are present in the merge commit since they come from 712c266 which is not included in your base branch. If you rebase your branch on top of the latest main, you should see them.

[sbfnk]

Thanks but these are no longer present in 9953900 which failed the checks. I'm confused.

They are present in the merge commit since they come from 712c266 which is not included in your base branch. If you rebase your branch on top of the latest main, you should see them.

Thanks - I had no idea action were checking merge commits.