TuringLang / ParetoSmooth.jl

An implementation of PSIS algorithms in Julia.
http://turinglang.org/ParetoSmooth.jl/
MIT License
19 stars 12 forks source link

Updates for issue #24 #25

Closed goedman closed 3 years ago

goedman commented 3 years ago

Looks like the last pull request needs some minor edits:

Hmm, this isn't minor to me :-) Anyway, with the caveat that I am not a programmer, I just use Julia to get some things done, I think I have more or less addressed most of below issues.

1. The functions seem to be very overtyped, assuming that everything is a Vector{Float64}, while users may want to use a different data structure. (Big data can oftentimes only be held in Float32, Float16, or more esoteric Float types.) The only thing we can assume is that anything passed will be an AbstractVector containing some kind of Real number. (Can't even assume they're Floats -- ForwardDiff's dual numbers aren't classed as floats since they're internally represented as a tuple of floats, so they're only a subtype of Real.)

I believe this is covered now.

2. We need a comparison function that takes PsisLoo objects as inputs, rather than just a vector of log-likelihoods. I think it would be good to have functions dealing with the case that the user passes either a variable number of PsisLoo objects (without an enclosing data structure), or a named tuple of PsisLoo objects (in which case the names of the objects should be used as the model names).

Done.

3. Looks like there's typos in the docstrings, and general formatting errors in the code. We should try and adhere to the Blue style guide from Invenia.

I am not very familiar with this guide. I've skimmed through it but not sure adhere in all aspects.

_4. We should hold to the API for ParetoSmooth samples (e.g. using something similar to :loo_score instead of :elpddiff). The names were chosen to be generic, so we can implement other score functions besides elpd later. There also seems to be some information missing, like differences in the effective number of parameters.

This update was unfortunate as I posted it for a couple of days before applying the change. I'm also not familiar where you ultimately would like to go with this package.

I hesitate to add the parameter penalty difference column as I used the calculation method from Statistical Rethinking and did not want to corrupt the MIT license.

ParadaCarleton commented 3 years ago

Thank you so much @goedman !

goedman commented 3 years ago

@ParadaCarleton Your welcome. As always I learn something from these requests. In the end I struggled with 2 issues:

In addition to what I already mentioned on the parameter penalty, I’ve also been wondering how to do that with only PsisLoo objects. Otherwise, is that reason enough to carry the loglikelihood matrices along?

In a case like this, with 3 loo_compare methods I prefer to have a single doc entry, but that is probably a matter of preference.