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

New DD models #34

Open
wants to merge 58 commits into
base: develop
Choose a base branch
from
Open

New DD models #34

wants to merge 58 commits into from

Conversation

TheoPannetier
Copy link
Collaborator

@TheoPannetier TheoPannetier commented Sep 23, 2021

  • I expanded the possible of values of arguments ddmodel / ddep from 5 to 15 possible DD models, covering all combinations of linear, power, or exponential DD; or CR speciation / extinction (except of course the CR speciation + CR extinction model).

  • Updated the corresponding doc

  • Added tests to make sure that

    1. these new DD models return the expected value for both rates
    2. dd_loglik() doesn't crash and returns a non-infinite loglik when using them
    3. forbidden parameter values make dd_loglik() return -Inf instead of an error
  • Added a few helper functions (both_rates_vary(), is_speciation_linear(), get_Kprime()) to help with model-dependent conditions

Theo Pannetier added 30 commits March 29, 2021 17:11
Merge branch 'develop' into new-ddmodels

# Conflicts:
#	R/dd_loglik.R
#	tests/testthat/test_z_DDD.R
@TheoPannetier
Copy link
Collaborator Author

Something is going wrong with dd_ML() in this version, I'm investigating

The problem is with odeint solvers getting blocked when the gradient of probabilities becomes too small. This issue is present on develop and not specific to this branch, so I enable this PR again

R/dd_ML.R Show resolved Hide resolved
Comment on lines +401 to +403
#if (ddep > 5) {
# stop("This DD model is not implemented for the analytical method yet.")
#}
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
#if (ddep > 5) {
# stop("This DD model is not implemented for the analytical method yet.")
#}

Commented out code is not desirable

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only put stop for user-accessible functions

Comment on lines -184 to +436
abstol = 1e-10
reltol = 1e-8
abstol = 1e-16
reltol = 1e-10
Copy link
Collaborator Author

@TheoPannetier TheoPannetier Dec 5, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

keep reduced tolerance for CR model?

Comment on lines -215 to +466
y = dd_integrate(probs,brts[(k-1):k],rhs_func_name,c(pars1,k1,ddep),rtol = reltol,atol = abstol,method = methode)
probs = y[2,2:(lx+1)]
#y = deSolve::ode(probs,brts[(k-1):k],rhs_func,c(pars1,k1,ddep),rtol = reltol,atol = abstol,method = "analytical")
#probs2 = y[2,2:(lx+1)]
probs = dd_loglik_M(pars1,lx,k1,ddep,tt = abs(brts[k] - brts[k-1]),probs)
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm unsure which version should be kept here

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, this is just git mismatching the versions because of the large size of the file and the redundancies in the script. In reality, new-dd-models retains the same version as develop


if (ddmodel > 5) {
if (methode == "analytical" || cond == 3) {
stop("Sorry, ddmodel options > 5 have not been developed for method = \"analytical\" or cond = 3.")
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
stop("Sorry, ddmodel options > 5 have not been developed for method = \"analytical\" or cond = 3.")
stop("Sorry, ddmodel options > 5 have not been tested for method = \"analytical\" or cond = 3.")

@TheoPannetier
Copy link
Collaborator Author

Hi @rsetienne, are there still some standing issues we need to address here, or is this branch ready to be merged to develop?

@rsetienne
Copy link
Owner

rsetienne commented Feb 10, 2023 via email

@rsetienne
Copy link
Owner

There are still errors on GHA when trying to merge. @TheoPannetier, can you please check?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants