Open beanumber opened 5 months ago
Thanks for starting a discussion on this. There are a couple of reasons I avoided returning the logLik class:
I'm open to hearing rebuttal to these points.
Thanks for the quick response!
FWIW, I still think it would be better to:
logLik()
to conform to the standards outlined above. [See also the "Value" section of the documentation for logLik()
.] I guess I'm not sure what that would mean for backwards compatibility and how big of a deal that would be. likelihood()
with the current behavior (although I think objective_fun()
would be a more accurate name)With respect to your reasoning:
logLik()
and AIC()
. If these functions worked as expected as above, it would be more straightforward to do the kind of comparative analysis you're talking about. If you want the penalty value, and you're using AIC, then just run AIC(x)
and you get that value. In the current framework, it's logLik(x)[2]
, which is not so obvious. Also, if you want the actual log-likelihood, you have to compute -logLik(x)/2
, which is also weird. objective_fun()
(or whatever) that returns this value. I would also change the name
of the vector to be the name of the penalty function. So in the above case, instead of -2*Loglike+pen
the name would be AIC
(or mBIC
or whatever was appropriate). In general, I think function names should reflect what the function does as much as possible, and methods should work within the guidelines of their corresponding generic. So logLik()
should return a log-likelihood value (and corresponding class as defined in stats
), AIC()
should return an AIC value, and objective_fun()
should return the value of the objective function. [Generally, each function should probably return only one value.]
Thanks for considering this. If you're curious, this is the compatibility layer I've written for changepoint
in tidychangepoint
. So I have already have my workaround. But as per Hadley's advice, I'm trying to push the logLik.cpt()
method into changepoint
rather than overwriting it (bad manners!) in tidychangepoint
! :)
I'm open to changing this functionality, especially for logLik() as it is bad practice.
As a side note, no one should ever be using AIC() with changepoint models - it asymptotically recovers too many changepoints.
Thanks for indulging me. :)
FYI, I'm now using fitness()
as a generic method to return a named vector with the value of the objective function. So it works like this:
library(tidychangepoint)
x <- segment(CET, method = "pelt")
#> method: pelt
fitness(x)
#> MBIC
#> 23.56658
x$segmenter
#> Class 'cpt' : Changepoint Object
#> ~~ : S4 class containing 12 slots with names
#> cpttype date version data.set method test.stat pen.type pen.value minseglen cpts ncpts.max param.est
#>
#> Created on : Thu Jun 8 14:09:02 2023
#>
#> summary(.) :
#> ----------
#> Created Using changepoint version 2.2.4
#> Changepoint type : Change in mean and variance
#> Method of analysis : PELT
#> Test Statistic : Normal
#> Type of penalty : MBIC with value, 23.56658
#> Minimum Segment Length : 2
#> Maximum no. of cpts : Inf
#> Changepoint Locations : 55 57 309 311 330
Created on 2024-04-23 with reprex v2.1.0
Oh, but also, I just realized my computation is wrong, because fitness()
should return the value of the objective function, not just the penalty!
Thanks for the great work on this package!
The behavior of
changepoint::logLik.cpt()
is problematic for three reasons:double
instead of an object of classlogLik
, and thus the resulting object doesn't have the attributes thatlogLik
objects should have.This means that other generic functions already defined in
stats
likeAIC()
andBIC()
don't work as expected.Created on 2024-04-03 with reprex v2.1.0
I think it would be better if
changepoint::logLik.cpt()
returned alogLik
object with the appropriate attributes and values. Something like this should do the trick:Created on 2024-04-03 with reprex v2.1.0