Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Soss test (update of #62) #135

Merged
merged 14 commits into from
Apr 14, 2021
Merged

Soss test (update of #62) #135

merged 14 commits into from
Apr 14, 2021

Conversation

devmotion
Copy link
Member

I just updated #62 some days ago. However, unfortunately, sampling with Soss fails due to errors such as

  Test threw exception
  Expression: length(Soss.sample(DynamicHMCChain, m | (y = y,), 5)) == 5
  UndefVarError: testvalue not defined
  Stacktrace:
    [1] getproperty
      @ ./Base.jl:26 [inlined]
    [2] macro expansion
      @ ~/.julia/packages/GeneralizedGenerated/PV9u7/src/closure_conv.jl:132 [inlined]
    [3] _xform(M::Type{GeneralizedGenerated.NGG.TypeLevel{Module, "Buf{17}()"}}, _m::Soss.Model{NamedTule{(:X,), T} where T<:Tuple, GeneralizedGenerated.NGG.TypeLevel{Expr, "Buf{23}()"}, GeneralizedGenerate.NGG.TypeLevel{Module, "Buf{17}()"}}, _args::NamedTuple{(:X,), Tuple{Vector{Vector{Float64}}}}, _data::amedTuple{(:y,), Tuple{Vector{Int64}}})
      @ Soss ~/.julia/packages/GeneralizedGenerated/PV9u7/src/closure_conv.jl:132
    [4] xform(m::Soss.ConditionalModel{NamedTuple{(:X,), T} where T<:Tuple, GeneralizedGenerated.NGG.TyeLevel{Expr, "Buf{23}()"}, GeneralizedGenerated.NGG.TypeLevel{Module, "Buf{17}()"}, NamedTuple{(:X,), Tple{Vector{Vector{Float64}}}}, NamedTuple{(:y,), Tuple{Vector{Int64}}}})
      @ Soss ~/.julia/packages/Soss/PRbUF/src/primitives/xform.jl:23
    [5] sample(rng::Random._GLOBAL_RNG, ::Type{DynamicHMCChain}, m::Soss.ConditionalModel{NamedTuple{(:,), T} where T<:Tuple, GeneralizedGenerated.NGG.TypeLevel{Expr, "Buf{23}()"}, GeneralizedGenerated.NGG.ypeLevel{Module, "Buf{17}()"}, NamedTuple{(:X,), Tuple{Vector{Vector{Float64}}}}, NamedTuple{(:y,), Tupe{Vector{Int64}}}}, nsamples::Int64, nchains::Int64)
      @ Soss ~/.julia/packages/Soss/PRbUF/src/samplechains/dynamichmc.jl:15
    [6] sample (repeats 2 times)
      @ ~/.julia/packages/Soss/PRbUF/src/samplechains/dynamichmc.jl:29 [inlined]
    [7] macro expansion
      @ ~/.julia/dev/AbstractGPs/test/compat/soss.jl:49 [inlined]
    [8] macro expansion
      @ /buildworker/worker/package_linux64/build/usr/share/julia/stdlib/v1.6/Test/src/Test.jl:1151 [inined]
    [9] macro expansion
      @ ~/.julia/dev/AbstractGPs/test/compat/soss.jl:31 [inlined]
   [10] macro expansion
      @ /buildworker/worker/package_linux64/build/usr/share/julia/stdlib/v1.6/Test/src/Test.jl:1151 [inined]
   [11] top-level scope
      @ ~/.julia/dev/AbstractGPs/test/compat/soss.jl:2

Since I am new to Soss I am not completely sure but to me it seems this could be a bug in Soss? Or do I use it incorrectly @cscherrer?

test/compat/soss.jl Outdated Show resolved Hide resolved
test/compat/soss.jl Outdated Show resolved Hide resolved
test/compat/soss.jl Outdated Show resolved Hide resolved
test/compat/soss.jl Outdated Show resolved Hide resolved
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
test/runtests.jl Outdated Show resolved Hide resolved
@devmotion devmotion mentioned this pull request Apr 12, 2021
@cscherrer
Copy link
Contributor

Thanks @devmotion and @willtebbutt ! I'm sure there's just a missing method, and I'd love to have some more tests anyway. I'll have a look

@devmotion
Copy link
Member Author

An additional problem is that apparently SampleChainsDynamicHMC requires Julia 1.5.

@cscherrer
Copy link
Contributor

testvalue

This is defined in MeasureTheory. The semantics are to return a point in the support, like rand without the randomness. I use this in the process of determining the transform for the model. rand adds a little overhead, and requires a rand method that some measures don't have. Does Distributions have something like this? If not I could define

MeasureTheory.testvalue(d::Distributions.Distribution) = rand(d)

Julia 1.5
I'm not sure I need this in SampleChains etc, mostly I figured there's not much reason to not be using at least 1.5. I do require it in MeasureTheory for the (;x) trick it adds.

@cscherrer
Copy link
Contributor

I didn't realize I'm reexporting MeasureTheory from Soss, I'll need to change that to make Distributions usable. This is a breaking change, so we'll be at v0.18

@devmotion
Copy link
Member Author

I'm not sure I need this in SampleChains etc, mostly I figured there's not much reason to not be using at least 1.5.

This is quite unfortunate since there is no reason for us to drop support for Julia 1.3 and 1.4 at this point. Probably then we have to separate Soss and its dependencies into a separate environment if we want to test it, and make the tests optional.

@cscherrer
Copy link
Contributor

I'll try things out with 1.3, maybe it works or could be made to work without much trouble

test/compat/soss.jl Outdated Show resolved Hide resolved
test/compat/soss.jl Outdated Show resolved Hide resolved
@cscherrer
Copy link
Contributor

Getting closer! I'm now at

julia> include("test/compat/soss.jl")
latent GP regression: Error During Test at /home/chad/git/AbstractGPs.jl/test/compat/soss.jl:45
  Test threw exception
  Expression: length(Soss.sample(DynamicHMCChain, m | (y = y,), 5)) == 5
  Not implemented:
  xform(FiniteGP{GP{ZeroMean{Float64}, Matern32Kernel}, RowVecs{Float64, Matrix{Float64}, SubArray{Float64, 1, Matrix{Float64}, Tuple{Int64, Base.Slice{Base.OneTo{Int64}}}, true}}, Diagonal{Float64, Fill{Float64, 1, Tuple{Base.OneTo{Int64}}}}}(
  f: GP{ZeroMean{Float64}, Matern32Kernel}(ZeroMean{Float64}(), Matern 3/2 Kernel)
  x: SubArray{Float64, 1, Matrix{Float64}, Tuple{Int64, Base.Slice{Base.OneTo{Int64}}}, true}[[1.284670749722735], [-0.48905837672741387], [1.234744953422099]]
  Σy: [1.0e-18 0.0 0.0; 0.0 1.0e-18 0.0; 0.0 0.0 1.0e-18]
  )
  )

This means Soss needs to know how to transform a Gaussian process to R^n, so we'll need to think about the right way to set this up. The obvious approach is just to make AbstractGPs a dependency of Soss (probably with @require, but I'd rather find a simpler way if there is one. I'm mostly using TransformVariables for this.

@devmotion
Copy link
Member Author

This means Soss needs to know how to transform a Gaussian process to R^n, so we'll need to think about the right way to set this up. The obvious approach is just to make AbstractGPs a dependency of Soss (probably with @require, but I'd rather find a simpler way if there is one. I'm mostly using TransformVariables for this.

IMO it should just work since FiniteGP is a subtype of Distributions.AbstractMvNormal. There is no need to depend on AbstractGPs.

@devmotion
Copy link
Member Author

Regarding your previous question:

Does Distributions have something like this?

No, there's nothing like this in Distributions (at least I am not aware of anything). Probably the safest option is to sample from the distribution.

@devmotion devmotion requested review from willtebbutt and theogf April 14, 2021 21:42
Copy link
Member

@willtebbutt willtebbutt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is great. I just have one question that I don't want to prevent you from merging.

Regarding the GROUP environment variable, do we ever set it to anything other than All or AbsrtactGPs? I'm just wondering whether there are actually situation in which we just want to run the compat tests?

.github/workflows/ci.yml Outdated Show resolved Hide resolved
@devmotion
Copy link
Member Author

No, probably the "Compat" alternative could/should be removed.

Some background info:
Initially, I had only added a check of the Julia version (>= v"1.5") to avoid test failures on older versions that are not supported by Soss anymore while still being able to run our standard tests on Julia 1.3. However, I noticed that one also has to upper bound the Julia version - Turing depends on Libtask_jll which has to be built against libjulia which does not exist in Yggdrasil for unstable Julia versions. Due to this issue tests will fail on nightly (it works currently in the regular test setup since Pkg pulls in an old Turing version that did not depend on Libtask_jll, in the separate project environment it is resolved differently). To fix these issues one could check for Julia 1.6 explicitly (== v"1.6") but then one would have to update the check manually for new stable versions (otherwise these tests are just skipped). I introduced the environment variable to ensure that Soss and Turing are always tested with the stable Julia version (the version check is included to prevent anyone, also offline, from trying to run the tests with older Julia versions).

@devmotion
Copy link
Member Author

It also seems that sometimes Soss/DynamicHMC fails due to numerical errors. I assume that the variance can become too small, either in the warmup phase or during sampling. I'll check if it is sufficient to add some small positive offset (similar to what we do when plotting samples).

@codecov-io
Copy link

Codecov Report

Merging #135 (8164dd1) into master (c06c892) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #135   +/-   ##
=======================================
  Coverage   99.22%   99.22%           
=======================================
  Files           9        9           
  Lines         259      259           
=======================================
  Hits          257      257           
  Misses          2        2           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c06c892...8164dd1. Read the comment docs.

@devmotion devmotion merged commit 869b738 into master Apr 14, 2021
@devmotion devmotion deleted the pr-62 branch April 14, 2021 22:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants