Bugfix in VarInfo.
#516
Bugfix in VarInfo.
#516
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
I'll have a look at this in a few hours. But is it not supposed to be Also is this there an example test/case for this bug? As in, what is this fixing, given that we didn't have any failing tests in previous PRs? |
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Fixed.
It fixes #504 (comment). Not sure why our tests didn't catch this issue -- maybe we didn't have a test targeting this new case at all. |
Pull Request Test Coverage Report for Build 5801165167
💛 - Coveralls |
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## master #516 +/- ##
=======================================
Coverage 76.35% 76.36%
=======================================
Files 24 24
Lines 2770 2771 +1
=======================================
+ Hits 2115 2116 +1
Misses 655 655
☔ View full report in Codecov by Sentry. |
Co-authored-by: David Widmann <[email protected]>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
|
I added some tests. Feel free to merge when CI passes. |
test/varinfo.jl
Outdated
| @model function demo2d() | ||
| return x ~ Dirichlet(2, 1.0) | ||
| end |
There was a problem hiding this comment.
| @model function demo2d() | |
| return x ~ Dirichlet(2, 1.0) | |
| end | |
| @model demo2d() = x ~ Dirichlet(2, 1.0) |
test/varinfo.jl
Outdated
| @model function demo3d() | ||
| return x ~ Dirichlet(3, 1.0) # increase K to 3 | ||
| end |
There was a problem hiding this comment.
| @model function demo3d() | |
| return x ~ Dirichlet(3, 1.0) # increase K to 3 | |
| end | |
| @model demo3d() = x ~ Dirichlet(3, 1.0) |
test/varinfo.jl
Outdated
| @@ -273,6 +273,40 @@ | |||
| @test vals_prev == vi.metadata.x.vals | |||
| end | |||
|
|
|||
| # See https://github.com/TuringLang/DynamicPPL.jl/issues/504 | |||
| @testset "Dimentionality checks" begin | |||
There was a problem hiding this comment.
Can we move this to the same tests as added in #513, i.e. the testset in test/linking.jl + include tests for all the varinfos (those tests only test for 2d dirichlet, so it's just a matter of adding one more model to test).
test/varinfo.jl
Outdated
| end | ||
| model = demo2d() | ||
| vi = VarInfo(model) # make VarInfo -> sample from prior and compute logdensity | ||
| getlogp(vi) ≈ 0.0 # zero because Dirichlet(1) == Uniform over Simplex |
test/varinfo.jl
Outdated
| getlogp(vi) ≈ 0.0 # zero because Dirichlet(1) == Uniform over Simplex | ||
| spl = SampleFromPrior() # create dummy sampler for linking | ||
| DynamicPPL.link!!(vi, spl, model) # transform to unconstrained space | ||
| !(0.0 ≈ getlogp(last(DynamicPPL.evaluate!!(model, vi)))) # non-zero now due to log(abs(determinant(jacobian))) |
test/varinfo.jl
Outdated
| end | ||
| model = demo3d() | ||
| vi = VarInfo(model) # make VarInfo -> sample from prior and compute logdensity | ||
| getlogp(vi) ≈ 0.0 # zero because Dirichlet(1) == Uniform over Simplex |
test/varinfo.jl
Outdated
| getlogp(vi) ≈ 0.0 # zero because Dirichlet(1) == Uniform over Simplex | ||
| spl = SampleFromPrior() # create dummy sampler for linking | ||
| DynamicPPL.link!!(vi, spl, model) # transform to unconstrained space | ||
| !(0.0 ≈ getlogp(last(DynamicPPL.evaluate!!(model, vi)))) # non-zero now due to log(abs(determinant(jacobian))) |
|
I'll probably not have much time till late this week; please feel free to take over this PR if it needs to be merged quicker. |
|
Fixed the tests in #517 ; feel free to just merge this into this PR. Then it should be an easy approve:) (the added tests are passing locally) |
No description provided.