ParetoSmooth.jl
ParetoSmooth.jl copied to clipboard
TuringLang
Fix handling of unconventional variable names
If you have a variable named something["z"][()] the loo* functions will fail, because it tries to split, but will get the wrong part, changing this to rsplit and limiting the number of elemetns to 2 resolves this and will always take the rightmost [] which contains the index.
I am not familiar with regular expressions. @sunxd3 / @penelopeysm, can you take a look?
@zeyus this looks good, thanks a lot for the contribution.
Just for the ease of future maintenance, maybe we can move the function ind_from_string out of ParetoSmooth.pointwise_log_likelihoods and write some doctest?
IIRC there are quite a few suboptimal implementations in ParetoSmooth (even a few more in the function alone that is touched by this PR), but I haven't really used the package a lot, so there hasn't been a strong incentive from my side to improve things. IMO it would be good to make the code more robust and better tested. Or maybe just join forces with https://github.com/arviz-devs/PSIS.jl which (AFAIK) has better code quality and is better maintained.
Regarding this PR more concretely: Can't we avoid parsing strings completely and just work with VarNames (as shown in https://github.com/TuringLang/DynamicPPL.jl/blob/3b8dceab76ef37d57d0abdecb205a5c46550679f/src/loglikelihoods.jl#L201-L206)? And do we actually have to sort, given that (nowadays) DynamicPPL.pointwise_loglikelihoods returns an OrderedDict?
Regarding this PR more concretely: Can't we avoid parsing strings completely and just work with
VarNames (as shown in https://github.com/TuringLang/DynamicPPL.jl/blob/3b8dceab76ef37d57d0abdecb205a5c46550679f/src/loglikelihoods.jl#L201-L206)? And do we actually have to sort, given that (nowadays)DynamicPPL.pointwise_loglikelihoodsreturns anOrderedDict?
I'm happy to change to this if that's the better option. I just made the PR based on the minor change I made to get it working. I'm not entirely sure yet how much work it would be to convert it to using VarName as I'm not familiar with the interface.
IIRC there are quite a few suboptimal implementations in ParetoSmooth (even a few more in the function alone that is touched by this PR), but I haven't really used the package a lot, so there hasn't been a strong incentive from my side to improve things. IMO it would be good to make the code more robust and better tested. Or maybe just join forces with https://github.com/arviz-devs/PSIS.jl which (AFAIK) has better code quality and is better maintained.
Sorry, slightly off-topic response. There were early attempts to unify ParetoSmooth and PSIS (offline discussions and #41, #42, #43, #44, #51), but it didn't go anywhere because I'm quite pleased with the PSIS.jl API and code quality, and it would be more work to unify the packages than it was to write PSIS.jl to begin with.
There's also the difference in scope; PSIS.jl is only an ultra-lightweight implementation of the PSIS method designed to be used by downstream packages (e.g. any VI package could use this; Pathfinder.jl uses it as a diagnostic). LOO is implemented in PosteriorStats.jl. PSIS.jl and PosteriorStats.jl provide no integration with PPLs or objects that store MCMCChains and won't in the future, being deliberately PPL-agnostic. PPL maintainers are expected to add integrations to their own packages, so that they can ensure these integrations are kept up-to-date. (e.g. InferenceObjects integrates with PosteriorStats, and MCMCChains will as well once I finish https://github.com/TuringLang/MCMCChains.jl/issues/430). For loo or compare with Model inputs, DynamicPPL would need to add a PosteriorStats integration.
@sethaxen, apart from the integration with PPLs, are there any other differences between ParetoSmooth and PSIS?
@sethaxen, apart from the integration with PPLs, are there any other differences between ParetoSmooth and PSIS?
It's been a long time since I made a comparison, but besides the differences mentioned in https://github.com/TuringLang/ParetoSmooth.jl/pull/97#issuecomment-2296668010 (scope, package weight, API, code quality, test coverage), there are a few differences in functionality:
- they use different ESS estimates. ParetoSmooth's is more pessimistic, while PSIS's uses the one from the paper, which is connected with MCSE like the usual ESS.
- PSIS also includes a diagnostic plot recipe: https://julia.arviz.org/PSIS/stable/plotting/
- PosteriorStats includes Pseudo-BMA, bootstrapped Pseudo-BMA, and stacking model weighting methods for model comparison (see https://julia.arviz.org/PosteriorStats/stable/api/#Model-comparison). Stacking is currently the recommended approach in the literature. IIRC ParetoSmooth only implements Pseudo-BMA.
- PosteriorStats also includes
loo_pitfor posterior predictive checks: https://julia.arviz.org/PosteriorStats/stable/api/#PosteriorStats.loo_pit
Both packages are outdated in their use of the diagnostics. PSIS is shortly getting updates from the recent PSIS paper revision.
But is it clear whether the sorting is actually needed? https://github.com/TuringLang/ParetoSmooth.jl/pull/97#issuecomment-2285464006
@devmotion if I understand your point correctly, are you suggesting we should give some kind of ordering to VarNames?
Without a isless, OrderedDict should still work (it just use the insertion order as the order), but if there is a need to do sort!(d::OrderedDict; kwargs), then we definitely need to add a isless implementation.
OrderedDict for pointwise_loglikelihoods was introduced in DynamicPPL 0.22.4 (https://github.com/TuringLang/DynamicPPL.jl/pull/475), and the compat bounds for DynamicPPL here go all the way down to 0.21, so I guess technically the sorting is still needed. Whether the compat bound is accurate is another issue, (as are perhaps a few other things in this function - having worked on Turing for a while now I totally agree that we should use VarName) but since this is a strict improvement on the current, and nobody else is really working on it, I think I'm happy to merge this.
