TuringLang / TuringGLM.jl

Bayesian Generalized Linear models using `@formula` syntax.
https://turinglang.org/TuringGLM.jl/dev
MIT License
70 stars 7 forks source link

Remove in-place updates and fix return values #64

Closed devmotion closed 1 year ago

devmotion commented 1 year ago

Currently, some models perform in-place updates which breaks ReverseDiff support (#58) and return values that do not exist (if isempty(intercept_ranef)). Both issues are fixed by the PR.

Probably it would be good to add some tests.

Fixes #58.

storopoli commented 1 year ago

I will add the tests. Thanks for the PR! One thing: why are we returning nothing now? What if the user wants to perform posterior predictive checks?

devmotion commented 1 year ago

Because currently the return values are broken. Some of the to-be-returned parameters don't always exist since they are not sampled if isempty(intercept_randef). Alternatively, we could fix that by return different named tuples, depending on whether they exist or not, but then the model function becomes unstable. So with the current setup the simplest thing to do that ensures type stability and that return values actually exist is to return nothing.

I'm not sure how you usually perform posterior predictive checks. But e.g. using rand(model) will just sample a NamedTuple (or maybe Tor changed it to a Dìct already since NamedTuple does not work as generally IIRC) of the variables, so it's not necessary to explicitly return a named tuple of variables in general. For posterior predictive checks, I think, it would also be useful to use the conditioning syntax but that's probably best left for another PR.

storopoli commented 1 year ago

@devmotion you know what might be causing this with Tracker:

Not implemented: convert tracked Tracker.TrackedReal{Float64} to tracked Float64

https://github.com/TuringLang/TuringGLM.jl/runs/8196333415?check_suite_focus=true#step:6:721

storopoli commented 1 year ago

Zygote also fails with:

Zygote: Error During Test at /home/runner/work/TuringGLM.jl/TuringGLM.jl/test/runtests.jl:41
  Got exception outside of a @test
  Can't differentiate foreigncall expression $(Expr(:foreigncall, :(Core.tuple("pt", StatsFuns.RFunctions.libRmath)), Float64, svec(Float64, Float64, Int32, Int32), 0, :(:ccall), %11, %12, %13, %14, %10, %9, %8, %7)).
  You might want to check the Zygote limitations documentation.
  https://fluxml.ai/Zygote.jl/latest/limitations
devmotion commented 1 year ago

Yep, that's expected when you work with eg cdfs of t distributions in Julia. It's one of the few cases that still call out into Rmath, and hence it can't be differentiated by Zygote without a custom rule.

storopoli commented 1 year ago

Ok, we can merge this so that we support ReverseDiff and FowardDiff.