TuringLang / ParetoSmooth.jl

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

Add LOO capability #7

Closed ParadaCarleton closed 3 years ago

ParadaCarleton commented 3 years ago

@goedman @itsdfish Mostly done, but there's a couple things --

  1. If you guys know how to manipulate StructArrays, I'm having trouble getting the tests on lines 53+54 to run. I verified that they look good "By hand" but in the future it would be better for them to run automatically.
  2. I definitely don't like that Loo currently returns a weirdly structured Dict. I'd much prefer some kind of table.
  3. The Loo object needs some kind of nice-looking display method, possibly using PrettyTables.jl.
  4. More documentation would be nice.
ParadaCarleton commented 3 years ago

Also, there seems to be some kind of problem with running the tests on Github, even though they run fine locally. Not sure how to fix this given I'm not used to GitHub.

itsdfish commented 3 years ago

I will look at the StructArray issue tomorrow. In the meantime, the error report seems to indicate that Coveralls failed to send a message. I don't know how to setup Coveralls. If Rob is not familiar, the easiest solution in the short term is to disable it.

itsdfish commented 3 years ago

@ParadaCarleton and @goedman

I think part of the problem with the tests is that the data structures are different between R and Julia. In addition, there seems to be different information. This requires manually aligning data from the two sources. Here is the best I could do:

    # fails on pareto_k
    indices = [1,3,4]
    field_names = [:estimate,:p_eff,:pareto_k]
    for (i,f) in zip(indices,field_names)
         @test getproperty(jul_loo.pointwise, f) ≈ r_loo["pointwise"][:,i] rtol = .001
         @test getproperty(rel_eff_loo.pointwise, f) ≈ r_loo["pointwise"][:,i] rtol = .001
    end

    @test jul_loo.estimates["Score Est"] ≈  r_loo["estimates"][1] rtol = .001
    @test jul_loo.estimates["Parameters (Eff)"] ≈  r_loo["estimates"][2] rtol = .001
    @test rel_eff_loo.estimates["Score Est"] ≈  r_loo["estimates"][1] rtol = .001
    @test rel_eff_loo.estimates["Parameters (Eff)"] ≈  r_loo["estimates"][2] rtol = .001

The pointwise data fail on pareto_k. Those appear to be very different. :estimate and :p_eff pass. Since I am not familiar with the package and the details of LOO, I may have missed something. Please double check that I aligned the data properly.

ParadaCarleton commented 3 years ago

@itsdfish @goedman I've fully added LOO capability. The answers look good on inspection; however, a couple of the tests are broken and should probably be fixed before merge.

goedman commented 3 years ago

Hi @ParadaCarleton Thanks for all your work. I've been trying to figure out an issue in DiffEqBayes hence the silence from my side. But I'll go back to it asap. I had, probably on Monday, some trouble to compute a loo value which should be comparable to the WAIC.

ParadaCarleton commented 3 years ago

I've gotten the tests to work now; the only problem left to fix is that the MCSE calculations are way off, 2 or 3 times the amount they should be, and I can't figure out what the MCSE calculations in the R package are doing at all.

goedman commented 3 years ago

Hi Carlos, do you see real advantages to have a Project.toml file (and the associated Manifest.toml) in the test directory?

I've mostly seen the specific test dependencies being included in the [extras] section of the main Project.toml but maybe my workflow is outdated. I have also several times been bitten by storing manifest.toml files in a package in the repo.

In the current format, how and where do you run the Package tests? Simply (with ParetoSmooth dev-ed):

cd(".julia/dev/ParetoSmooth")
]
activate .
instantiate
test

fails on my system with:

  [8e850ede] nghttp2_jll v1.41.0+1 `@stdlib/nghttp2_jll`
  [3f19e933] p7zip_jll v16.2.1+1 `@stdlib/p7zip_jll`
     Testing Running tests...
WARNING: Wrapping `Vararg` directly in UnionAll is deprecated (wrap the tuple instead).
ERROR: LoadError: ArgumentError: No file exists at given path: test/Example_Log_Likelihood_Array.RData

Edit: Oops, forgot to checkout the loo branch and switched back to Julia 1.6. Now test works (well, similar as on CI).

ParadaCarleton commented 3 years ago

Hi Carlos, do you see real advantages to have a Project.toml file (and the associated Manifest.toml) in the test directory?

I've mostly seen the specific test dependencies being included in the [extras] section of the main Project.toml but maybe my workflow is outdated. I have also several times been bitten by storing manifest.toml files in a package in the repo.

Not sure; is there some other way to do it? It was set up automatically like this for me.

ParadaCarleton commented 3 years ago

And yes, all the tests should be working apart from the MCSE test, since I haven't been able to get MCSE calculations to work.

goedman commented 3 years ago

I really like what you have done with AxisKeys.jl and the KeyedArray objects. Had to step over my aversion against AxisArrays.jl which I always found very user unfriendly.

Guess with the test stuff, I will just have to update my workflow notions. Most packages added the test dependencies to the main Project.jl. But I can see this is a good approach and I've seen something about being able to test selections of all tests.

I'll set some time aside to see if I understand what's going on with the MCSE calculation.

ParadaCarleton commented 3 years ago

All tests are passing, so this looks good to go. MCSE gives somewhat different answers different from R (~ 30% RMSE), but they seem to be using a different algorithm from the one outlined in their paper; I've decided to just use the simpler algorithm in the paper for now.