[Merged by Bors] - Perform invlinking in assume rather than implicitly in getindex by torfjelde · Pull Request #360 · TuringLang/DynamicPPL.jl

torfjelde · 2022-01-07T21:06:44Z

Currently, in assume, etc., invlink is called implicitly in getindex using the distribution extracted from vi.

This has a couple of drawbacks:

We can only use the distribution for a particular vn stored in vi obtained during the initial run. This means that we can't even run models where the distributions has dynamic domains, i.e. the domain of a particular random variable is dependent on the realizations of other random variables.
We have to store the distribution for each vn in vi. This was fine when we only had VarInfo because we also need it for other functionality, but this is not the case in SimpleVarInfo (nor will it be).

So. In this PR we introduce a getindex_raw which is getindex but without invlink if it's already linked, and uses this within assume, etc. where we now use the distributions that are passed to assume rather than those stored in vi.

E.g. the following now works:

julia> @model demo() = x ~ InverseGamma(2, 3)
demo (generic function with 2 methods)

julia> vi = SimpleVarInfo((x = 10.0, ), true)
SimpleVarInfo((x = 10.0,), 0.0, true)

julia> _, vi = DynamicPPL.evaluate!!(model, vi, DefaultContext())
(22026.465794806718, SimpleVarInfo{NamedTuple{(:x,), Tuple{Float64}}, Float64}((x = 10.0,), -17.80291162245307, true))

devmotion · 2022-01-07T21:16:15Z

Maybe getindex should be getindex_raw and indexing with linking should use a different notation?

torfjelde · 2022-01-07T22:31:12Z

Maybe getindex should be getindex_raw and indexing with linking should use a different notation?

Ideally this is indeed how we do it. The current impl is super-confusing, e.g. if you use a sampler in getindex it will not transform the variables, and setindex expects values to be in constrained space, irregardless of whether you're indexing using a VarName or Sampler.

But I assume we need to do a bit of tracking down of all references to vi[vn] and ensure that we're not breaking anything in downstream packages, etc. Therefore my intention was to first just get a quick and dirty impl in that addresses the points mentioned in this PR, and then we make the indexing behavior consistent in a separate one.

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

torfjelde · 2022-01-27T13:51:32Z

bors try

bors · 2022-01-27T13:56:30Z

try

Build failed:

test (1, ubuntu-latest, x64, 2)

torfjelde · 2022-01-27T14:45:08Z

bors try

bors · 2022-01-27T15:00:42Z

try

Build failed:

test (1.3, ubuntu-latest, x64, 2)

torfjelde · 2022-01-27T15:11:15Z

Tests are failing on Julia 1.3 because the fix from TuringLang/Turing.jl#1758 hasn't made it through yet.

(It doesn't fail on Julia#latest because it's Julia 1.7 which makes it incompatible with latest release of Turing)

EDIT: It's passing locally using Turing#master on Julia 1.6.

yebai · 2022-01-27T15:15:18Z

@torfjelde it’s ok to make a new release from Turing#master.

…ations

bors · 2022-07-21T12:19:05Z

Canceled.

torfjelde · 2022-07-21T12:19:34Z

bors r+

@model

Currently, in `assume`, etc., `invlink` is called implicitly in `getindex` using the distribution extracted from `vi`. This has a couple of drawbacks: 1. We can only use the distribution for a particular `vn` stored in `vi` obtained during the initial run. This means that we can't even run models where the distributions has dynamic domains, i.e. the domain of a particular random variable is dependent on the realizations of other random variables. 2. We have to store the distribution for each `vn` in `vi`. This was fine when we only had `VarInfo` because we also need it for other functionality, but this is not the case in `SimpleVarInfo` (nor will it be). So. In this PR we introduce a `getindex_raw` which is `getindex` but without `invlink` if it's already linked, and uses this within `assume`, etc. where we now use the distributions that are passed to `assume` rather than those stored in `vi`. E.g. the following now works: ``` julia julia> @model demo() = x ~ InverseGamma(2, 3) demo (generic function with 2 methods) julia> vi = SimpleVarInfo((x = 10.0, ), true) SimpleVarInfo((x = 10.0,), 0.0, true) julia> _, vi = DynamicPPL.evaluate!!(model, vi, DefaultContext()) (22026.465794806718, SimpleVarInfo{NamedTuple{(:x,), Tuple{Float64}}, Float64}((x = 10.0,), -17.80291162245307, true)) ``` Co-authored-by: Hong Ge <hg344@cam.ac.uk>

bors · 2022-07-21T16:20:55Z

Timed out.

yebai · 2022-07-21T16:34:15Z

bors r+

@model

Currently, in `assume`, etc., `invlink` is called implicitly in `getindex` using the distribution extracted from `vi`. This has a couple of drawbacks: 1. We can only use the distribution for a particular `vn` stored in `vi` obtained during the initial run. This means that we can't even run models where the distributions has dynamic domains, i.e. the domain of a particular random variable is dependent on the realizations of other random variables. 2. We have to store the distribution for each `vn` in `vi`. This was fine when we only had `VarInfo` because we also need it for other functionality, but this is not the case in `SimpleVarInfo` (nor will it be). So. In this PR we introduce a `getindex_raw` which is `getindex` but without `invlink` if it's already linked, and uses this within `assume`, etc. where we now use the distributions that are passed to `assume` rather than those stored in `vi`. E.g. the following now works: ``` julia julia> @model demo() = x ~ InverseGamma(2, 3) demo (generic function with 2 methods) julia> vi = SimpleVarInfo((x = 10.0, ), true) SimpleVarInfo((x = 10.0,), 0.0, true) julia> _, vi = DynamicPPL.evaluate!!(model, vi, DefaultContext()) (22026.465794806718, SimpleVarInfo{NamedTuple{(:x,), Tuple{Float64}}, Float64}((x = 10.0,), -17.80291162245307, true)) ``` Co-authored-by: Hong Ge <hg344@cam.ac.uk>

bors · 2022-07-21T21:44:46Z

Build failed:

Turing.jl

This reverts commit 6f9be0d.

…o deprecations" This reverts commit 947e5c6.

torfjelde · 2022-07-22T09:29:27Z

bors r+

@model

Currently, in `assume`, etc., `invlink` is called implicitly in `getindex` using the distribution extracted from `vi`. This has a couple of drawbacks: 1. We can only use the distribution for a particular `vn` stored in `vi` obtained during the initial run. This means that we can't even run models where the distributions has dynamic domains, i.e. the domain of a particular random variable is dependent on the realizations of other random variables. 2. We have to store the distribution for each `vn` in `vi`. This was fine when we only had `VarInfo` because we also need it for other functionality, but this is not the case in `SimpleVarInfo` (nor will it be). So. In this PR we introduce a `getindex_raw` which is `getindex` but without `invlink` if it's already linked, and uses this within `assume`, etc. where we now use the distributions that are passed to `assume` rather than those stored in `vi`. E.g. the following now works: ``` julia julia> @model demo() = x ~ InverseGamma(2, 3) demo (generic function with 2 methods) julia> vi = SimpleVarInfo((x = 10.0, ), true) SimpleVarInfo((x = 10.0,), 0.0, true) julia> _, vi = DynamicPPL.evaluate!!(model, vi, DefaultContext()) (22026.465794806718, SimpleVarInfo{NamedTuple{(:x,), Tuple{Float64}}, Float64}((x = 10.0,), -17.80291162245307, true)) ``` Co-authored-by: Hong Ge <hg344@cam.ac.uk>

bors · 2022-07-22T11:22:30Z

Build failed:

Turing.jl

torfjelde · 2022-07-23T09:59:25Z

Unfortunately integration tests will fail because the tests for ESS are making assumptions about the DEMO_MODELS that are no longer true. As those are indeed the only tests that are failing, hence I suggest we just merge this and then update the tests in Turing accordingly (I have a working update locally).

In addition, I've bumped the minimum supported Julia version to 1.6 for DPPL; 1.6 is now LTS and we've already done this for Turing.jl, so continue to test against 1.3 but not 1.6 seems strange.

torfjelde · 2022-07-23T10:00:11Z

bors r+

@model

Currently, in `assume`, etc., `invlink` is called implicitly in `getindex` using the distribution extracted from `vi`. This has a couple of drawbacks: 1. We can only use the distribution for a particular `vn` stored in `vi` obtained during the initial run. This means that we can't even run models where the distributions has dynamic domains, i.e. the domain of a particular random variable is dependent on the realizations of other random variables. 2. We have to store the distribution for each `vn` in `vi`. This was fine when we only had `VarInfo` because we also need it for other functionality, but this is not the case in `SimpleVarInfo` (nor will it be). So. In this PR we introduce a `getindex_raw` which is `getindex` but without `invlink` if it's already linked, and uses this within `assume`, etc. where we now use the distributions that are passed to `assume` rather than those stored in `vi`. E.g. the following now works: ``` julia julia> @model demo() = x ~ InverseGamma(2, 3) demo (generic function with 2 methods) julia> vi = SimpleVarInfo((x = 10.0, ), true) SimpleVarInfo((x = 10.0,), 0.0, true) julia> _, vi = DynamicPPL.evaluate!!(model, vi, DefaultContext()) (22026.465794806718, SimpleVarInfo{NamedTuple{(:x,), Tuple{Float64}}, Float64}((x = 10.0,), -17.80291162245307, true)) ``` Co-authored-by: Hong Ge <hg344@cam.ac.uk>

torfjelde · 2022-07-23T10:02:05Z

@yebai Can you just merge this once Bors comes back with failure, assuming the ESS tests are the only ones failing?

torfjelde · 2022-07-23T10:05:03Z

Actually, given that we bump the lower bound for the Julia version, we should bump minor rather than patch version. This will also make bors happy 👍

bors · 2022-07-23T10:07:08Z

Canceled.

torfjelde · 2022-07-23T10:07:15Z

bors r+

@model

Currently, in `assume`, etc., `invlink` is called implicitly in `getindex` using the distribution extracted from `vi`. This has a couple of drawbacks: 1. We can only use the distribution for a particular `vn` stored in `vi` obtained during the initial run. This means that we can't even run models where the distributions has dynamic domains, i.e. the domain of a particular random variable is dependent on the realizations of other random variables. 2. We have to store the distribution for each `vn` in `vi`. This was fine when we only had `VarInfo` because we also need it for other functionality, but this is not the case in `SimpleVarInfo` (nor will it be). So. In this PR we introduce a `getindex_raw` which is `getindex` but without `invlink` if it's already linked, and uses this within `assume`, etc. where we now use the distributions that are passed to `assume` rather than those stored in `vi`. E.g. the following now works: ``` julia julia> @model demo() = x ~ InverseGamma(2, 3) demo (generic function with 2 methods) julia> vi = SimpleVarInfo((x = 10.0, ), true) SimpleVarInfo((x = 10.0,), 0.0, true) julia> _, vi = DynamicPPL.evaluate!!(model, vi, DefaultContext()) (22026.465794806718, SimpleVarInfo{NamedTuple{(:x,), Tuple{Float64}}, Float64}((x = 10.0,), -17.80291162245307, true)) ``` Co-authored-by: Hong Ge <hg344@cam.ac.uk>

bors · 2022-07-23T10:20:57Z

Pull request successfully merged into master.

Build succeeded:

performing linking in assume rather than implicitly in getindex

14594d6

github-actions Bot reviewed Jan 7, 2022

View reviewed changes

Comment thread src/varinfo.jl Outdated

Comment thread src/varinfo.jl Outdated

Comment thread test/model.jl Outdated

Comment thread test/model.jl Outdated

added istrans to SimpleVarInfo

0bc279f

github-actions Bot reviewed Jan 7, 2022

View reviewed changes

Comment thread src/simple_varinfo.jl Outdated

torfjelde and others added 7 commits January 7, 2022 23:46

Apply suggestions from code review

81ee12e

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

added a comment

d39f87d

bump patch version

23f34cc

introduced settrans!!

d3ec108

added istrans(vi) and renamed all occurences of trans! to trans!!

81782c9

exclusively use settrans!! to set the istrans for SimpleVarInfo

12bfb42

removed usage of deprecated method in turing tests

c2c2417

torfjelde commented Jan 12, 2022

View reviewed changes

Comment thread src/simple_varinfo.jl Outdated

torfjelde added 3 commits January 13, 2022 19:03

added docstring to settrans!!

2e2cb5c

include istrans flag in type of SimpleVarInfo instead

f6c3fc4

deprecated settrans! in favour of settrans!!

d643a78

github-actions Bot reviewed Jan 27, 2022

View reviewed changes

Comment thread src/DynamicPPL.jl Outdated

Comment thread src/simple_varinfo.jl Outdated

torfjelde added 2 commits January 27, 2022 13:48

added some tests specifically for istrans

3cab7d9

formatting

8b870dc

github-actions Bot reviewed Jan 27, 2022

View reviewed changes

Comment thread src/DynamicPPL.jl Outdated

Comment thread src/simple_varinfo.jl Outdated

bors Bot added a commit that referenced this pull request Jan 27, 2022

Try #360:

e01c0ca

fixed bugs for ThreadSafeVarInfo

0b304db

bors Bot added a commit that referenced this pull request Jan 27, 2022

Try #360:

c5b8a7f

upper bound Distributions because tests are sooooo slow due to deprec…

947e5c6

…ations

Update bors.toml

6f9be0d

torfjelde added 3 commits July 22, 2022 10:23

Revert "Update bors.toml"

5c5b9ce

This reverts commit 6f9be0d.

Revert "upper bound Distributions because tests are sooooo slow due t…

e0797cc

…o deprecations" This reverts commit 947e5c6.

switch of deprecation warnings from integration tests for now

4b0e0e1

torfjelde added 2 commits July 23, 2022 10:55

bump supported Julia version to 1.6

5a73c87

added ability to filter varnames to check in TestUtils.test_sampler

bb43021

bump minor version

d5a48f8

bors Bot changed the title ~~Perform invlinking in assume rather than implicitly in getindex~~ [Merged by Bors] - Perform invlinking in assume rather than implicitly in getindex Jul 23, 2022

bors Bot closed this Jul 23, 2022

bors Bot deleted the tor/link-improvements branch July 23, 2022 10:21

Uh oh!

Conversation

torfjelde commented Jan 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

devmotion commented Jan 7, 2022

Uh oh!

Uh oh!

torfjelde commented Jan 7, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

torfjelde commented Jan 27, 2022

Uh oh!

bors Bot commented Jan 27, 2022

try

Uh oh!

torfjelde commented Jan 27, 2022

Uh oh!

bors Bot commented Jan 27, 2022

try

Uh oh!

torfjelde commented Jan 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yebai commented Jan 27, 2022

Uh oh!

bors Bot commented Jul 21, 2022

Uh oh!

torfjelde commented Jul 21, 2022

Uh oh!

bors Bot commented Jul 21, 2022

Uh oh!

yebai commented Jul 21, 2022

Uh oh!

bors Bot commented Jul 21, 2022

Uh oh!

torfjelde commented Jul 22, 2022

Uh oh!

bors Bot commented Jul 22, 2022

Uh oh!

torfjelde commented Jul 23, 2022

Uh oh!

torfjelde commented Jul 23, 2022

Uh oh!

torfjelde commented Jul 23, 2022

Uh oh!

torfjelde commented Jul 23, 2022

Uh oh!

bors Bot commented Jul 23, 2022

Uh oh!

torfjelde commented Jul 23, 2022

Uh oh!

bors Bot commented Jul 23, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

torfjelde commented Jan 7, 2022 •

edited

Loading

torfjelde commented Jan 27, 2022 •

edited

Loading