khabbazian / l1ou

Detection of evolutionary shifts in Ornstein-Uhlenbeck models
GNU General Public License v2.0
19 stars 7 forks source link

criterion arg in estimate_convergent_regimes #12

Closed eliotmiller closed 8 years ago

eliotmiller commented 8 years ago

This is really a two-problem issue: In the past I found that with some versions of the package pBIC worked, and with others AICc worked, but both didn't always work. I found that if I let l1ou decide what was default it would work. With the current version, when I did that I received the error message:

system.time(backward <- estimate_convergent_regimes(model=forward)) Error in match.arg(criterion) : 'arg' must be of length 1 Calls: system.time ... estimate_convergent_regimes -> estimate_convergent_regimes_surface -> match.arg

When I tried re-running it explicitly setting criterion to pBIC, I got this error message

system.time(backward <- estimate_convergent_regimes(model=forward, criterion="pBIC")) Error in cmp_model_score_CR(tr, Y, model$shift.configuration, regimes, : undefined criterion for convergent evolution. Calls: system.time ... estimate_convergent_regimes_surface -> cmp_model_score_CR

It appears to be running when I explicitly set it to AICc.

The second part of this problem is that the estimate_convergent_regimes function suggests that only AICc and pBIC are options, but the documentation refers back to configuration_ic, which has 5 options. It should be made clear in the definition of the function that those other options either are or not options (i.e. the part where the function is defined and the "default" for criterion is set to c("AICc", "pBIC") needs to be updated if those other 3 options are indeed possible.

Lastly, and slightly unrelatedly, there isn't much documentation given for the new argument "method", and it doesn't say what the default is. This should be made clear. (as should it be made clear what the default criterion is for all functions that use it--since it does choose something whether provided with one or not).

Thanks!

eliotmiller commented 8 years ago

Update: it also failed when setting criterion to AICc with:

system.time(backward <- estimate_convergent_regimes(model=forward, criterion="AICc")) Error in cmp_model_score_CR(tr, Y, model$shift.configuration, regimes, : undefined criterion for convergent evolution. Calls: system.time ... estimate_convergent_regimes_surface -> cmp_model_score_CR

cecileane commented 8 years ago

There’s still his issue about documentation (see selected text below). Will you work on adding to the R documentation, or would you like me to do it? Cécile

On Jul 15, 2016, at 9:28 AM, eliotmiller notifications@github.com<mailto:notifications@github.com> wrote:

The second part of this problem is that the estimate_convergent_regimes function suggests that only AICc and pBIC are options, but the documentation refers back to configuration_ic, which has 5 options. It should be made clear in the definition of the function that those other options either are or not options (i.e. the part where the function is defined and the "default" for criterion is set to c("AICc", "pBIC") needs to be updated if those other 3 options are indeed possible.

Lastly, and slightly unrelatedly, there isn't much documentation given for the new argument "method", and it doesn't say what the default is. This should be made clear. (as should it be made clear what the default criterion is for all functions that use it--since it does choose something whether provided with one or not).

khabbazian commented 8 years ago

The error has been fixed in version 1.23.