bbolker / bbmle

maximum likelihood estimation package
GNU General Public License v3.0
25 stars 13 forks source link

AIC should check for "inherits" instead of "class=" #11

Closed andrechalom closed 6 years ago

andrechalom commented 6 years ago

sads package defines two classes that inherit from mle2. Most methods work flawlessly on fitsad and fitrad classes, as method dispatching is done considering inheritance. However, the internal code for bbmle::AIC checks if all objects passed are of class mle2, which causes the following code to fail:

> fit1 = sads::fitlnorm(moths)
> class(fit1)
[1] "fitsad"
attr(,"package")
[1] "sads"
> inherits(fit1, "mle2")
[1] TRUE
> fit2 = sads::fitls(moths)
> AIC(fit1)
[1] 2199.445
> AIC(fit2)
[1] 2177.425
> AIC(fit1, fit2)
Error in AIC(fit1, fit2) : all objects in list must be class mle2

This can be fixed by changing the code in AIC to

if (!all(sapply(L, function(x) inherits(x, "mle2")))) 
    stop("all objects in list must be class mle2 or inherit from mle2")   
bbolker commented 6 years ago

I don't know why this restriction is here in the first place! I was going to blame it on previous code again, but it seems to be my fault.

My inclination is to remove it entirely and let the results fall back to stats:::AIC.default; I might spend a little more time thinking about why I did it this way in the first place/what might go wrong?

 dd <- data.frame(y=rpois(10,lambda=2))
> m1 <- glm(y~1,data=dd, family=poisson)
> m2 <- mle2(y~dpois(exp(logmu)),data=dd,start=list(logmu=0))
> AIC(m1,m2)
   df      AIC
m1  1 35.46634
m2  1 35.46634
> AIC(m2,m1)
Error in AIC(m2, m1) : all objects in list must be class mle2
> stats:::AIC.default(m2,m1)
   df      AIC
m2  1 35.46634
m1  1 35.46634
andrechalom commented 6 years ago

That's a fair point. I thought that this was in place to prevent errors like AIC(m1, m2, 2) instead of AIC(m1, m2, k=2), but that fails with a reasonably informative message anyway:

> AIC(m1, m2, 2)
Error in UseMethod("logLik") : 
  no applicable method for 'logLik' applied to an object of class "c('double', 'numeric')"
bbolker commented 6 years ago

this seems to be OK now. Close, or is a more fundamental fix necessary/useful?

andrechalom commented 6 years ago

Yes, I've just tested the development version of our package sads against the development version of bbmle, everything works as intended. I was waiting your input on "why I did it this way in the first place/what might go wrong?".