Closed sbfnk closed 8 months ago
Attention: 3 lines
in your changes are missing coverage. Please review.
Comparison is base (
d3e8d85
) 99.45% compared to head (6bf6e90
) 98.90%.:exclamation: 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 :warning: |
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
Might need updating in line with #190
LGTM. Thanks for the change. I believe it resolves #50 as well.
Might need updating in line with #190
Oh, I just merged that. You can rebase on main
.
LGTM. Thanks for the change. I believe it resolves #50 as well.
3 in one go!
Please feel free to rebase and merge.
@sbfnk Do you mind me merging this?
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 Do you mind me merging this?
I think @Bisaloo was still going to kindly review at some point in the near future.
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.
The branch needs to be rebased and the following examples updated:
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.
Does the action not check out a force pushed version?
Thanks but these are no longer present in https://github.com/epiverse-trace/epichains/commit/99539007407585f55c433e13959b2c780d651ae1 which failed the checks. I'm confused.
They are present in the merge commit since they come from https://github.com/epiverse-trace/epichains/commit/712c2661ca900fba24e4607838f337e0d46e1695 which is not included in your base branch. If you rebase your branch on top of the latest main
, you should see them.
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.
Please check if the PR fulfills these requirements
[X] I have read the CONTRIBUTING guidelines
[X] A new item has been added to
NEWS.md
[X] Tests for the changes have been added (for bug fixes / features)
[X] 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 oflikelihood
andsimulate_chains
to expect a function instead of a character string.Yes. No version has been released yet so not going through a deprecation cycle.
Doing this necessitated two further changes:
construct_offspring_ll_name
function and instead integrate the code intolikelihood
; because of the way the function name is extracted from theoffspring_dist
argument (usingsubstitute
) 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 lossrgborel
function; this is a difference from previous behaviour, where thell
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 correspondingll
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 anywayCloses #25 Closes #167