Open jamesmbaazam opened 8 months ago
Thanks for sharing!
I'm still struggling a little bit to wrap my head around the different outputs and the proposal here. A very helpful first step may be to provide examples that show all output types in likelihood()
documentation.
If I understand correctly the proposal here, it seems a bit redundant to have both the log and non log likelihood in the same object, especially when it's so fast to go from one to the other. Same for individual vs summed likelihoods.
I would probably suggest to be a bit more opinionated and indeed remove the individual
and log
argument, but stick to one set of values for these (e.g., always log = TRUE
& individual = TRUE
). This way, you get the benefits you mention here and keep a simpler and relatively stable output (just changing based on obs_prob == 1
).
This also relates to previous comments in #42 and #64. I'm not completely clear how much complexity this would add but I think that implementing S3 methods for predict
and logLik
would be potentially the best option. If I understand the logLik
class correctly then it's a vector with an attribute df
(the number of parameters estimated) which could be used here in line with @Bisaloo's comments.
likelihood()
has an EXTREMELY hard to read/understand return value, i.e.,I've thought about this and think we could return most of these in a named list and explain what each name means in the
@return
value section.Benefits:
individual
andlog
(because we'll just calculate the associated results)This is linked to #133.
@sbfnk @Bisaloo @chartgerink @pratikunterwegs Thoughts?