Fix prob macros #147

devmotion · 2020-06-28T00:13:18Z

This PR fixes #137 and #138, and allows to update a VarInfo object with a named tuple.

mohamed82008 · 2020-06-29T08:10:50Z

src/varinfo.jl

+    vi
+end
+_setval!(vi::TypedVarInfo, values, keys) = _typed_setval!(vi, vi.metadata, values, keys)
+@generated function _typed_setval!(


I think simple generated functions like this can be replaced by a map do-block on the named tuple directly. Last I checked the Julia compiler inferred and inlined it just fine with a do-block.

This can be done in many places in DPPL.

Yeah that's what I assumed as well but it's not true in general. At least in March I found the generated function to be much more efficient on a simple example (TuringLang/Turing.jl#1167 (comment)), and hence IMO one really has to benchmark every possible switch from generated function to regular map (which is a bit unfortunate since I'd like to just use regular functions wherever possible...).

I'll benchmark this example to see if we could get rid of the generated function here.

Probably ok to keep as-is here, and perform refactoring to change all places in another PR.

Is this one of those things where the compiler will "inline" everything up-to some fixed threshold? Same as with recursions. I remember looking into this stuff when working on Bijectors.jl, and there was essentially a fixed depth (I think ~20 or something) at which point the recursion (even though the methods were type-stable) wouldn't be unrolled. I bet it's the same here, where if names is sufficiently small then map will be the same as generated, but if names is large it won't since this can cause issues, e.g. a Turing model with millions of univariate parameters will probably not be too comfortable for the compiler. With that said, I agree with forcing "inlining" by the use of generated functions since if someone is running a model with millions of parameters, it's likely that you're still just looking at <30 different symbols.

devmotion · 2020-07-03T13:34:37Z

bors try

bors · 2020-07-03T13:35:00Z

try

Build failed:

test (1, ubuntu-latest, x86)

devmotion · 2020-07-03T13:35:31Z

bors try

bors · 2020-07-03T13:55:22Z

try

Build succeeded:

yebai · 2020-07-20T16:02:54Z

bors r+

bors · 2020-07-20T16:02:56Z

👎 Rejected by too few approved reviews

yebai · 2020-07-20T16:03:21Z

bors r+

This PR fixes #137 and #138, and allows to update a `VarInfo` object with a named tuple. Co-authored-by: David Widmann <[email protected]>

bors · 2020-07-20T16:17:02Z

Build failed:

test (1, ubuntu-latest, x64)

yebai · 2020-07-20T16:19:21Z

Test error seems related to TuringLang/Turing.jl#1358

torfjelde

Awesome stuff @devmotion !

I've added a couple of style-changes and a have a couple of questions (that might not be issues, but just me misunderstanding stuff!).

torfjelde · 2020-07-28T11:25:44Z

src/varinfo.jl

+    vi
+end
+_setval!(vi::TypedVarInfo, values, keys) = _typed_setval!(vi, vi.metadata, values, keys)
+@generated function _typed_setval!(


Is this one of those things where the compiler will "inline" everything up-to some fixed threshold? Same as with recursions. I remember looking into this stuff when working on Bijectors.jl, and there was essentially a fixed depth (I think ~20 or something) at which point the recursion (even though the methods were type-stable) wouldn't be unrolled. I bet it's the same here, where if names is sufficiently small then map will be the same as generated, but if names is large it won't since this can cause issues, e.g. a Turing model with millions of univariate parameters will probably not be too comfortable for the compiler. With that said, I agree with forcing "inlining" by the use of generated functions since if someone is running a model with millions of parameters, it's likely that you're still just looking at <30 different symbols.

src/varinfo.jl

src/prob_macro.jl

src/varinfo.jl

torfjelde · 2020-07-28T11:42:01Z

src/varinfo.jl

+) where {names}
+    updates = map(names) do n
+        quote
+            for vn in metadata.$n.vns


Isn't this always going to be just a single element now, which is the same as VarName($n)? If not, could you be so kind and explain? 👼 That was quite confusing to me when working on the other PR.

AFAIK metadata.$n.vns will be a proper vector of multiple elements as soon as the model contains random variables with different indexing, e.g., a vector-valued random variable x for which separate samples x[1] and x[2] are sampled in the generative model definition will lead to a TypedVarInfo object in which metadata is a NamedTuple with key :x and a corresponding value of type Metadata that contains all information of x[1] and x[2] - and hence a field vns that contains the VarName instances for both x[1] and x[2]. See

DynamicPPL.jl/src/varinfo.jl

Lines 490 to 498 in 275ccc8

"""

TypedVarInfo(vi::UntypedVarInfo)

This function finds all the unique `sym`s from the instances of `VarName{sym}` found in

`vi.metadata.vns`. It then extracts the metadata associated with each symbol from the

global `vi.metadata` field. Finally, a new `VarInfo` is created with a new `metadata` as

a `NamedTuple` mapping from symbols to type-stable `Metadata` instances, one for each

symbol.

"""

for the current implementation that creates TypedVarInfo objects from untyped VarInfo objects (usually after running the model once).

Ah, I see. So if x ~ MvNormal it won't be different, but if x[1] ~ Normal, x[2] ~ Normal then it will be?

Thanks man:)

Yes, exactly.

Co-authored-by: Tor Erlend Fjelde <[email protected]>

torfjelde · 2020-08-11T00:39:06Z

Is there anything keeping this from being merged?

mohamed82008 · 2020-08-11T01:28:06Z

bors r+

bors · 2020-08-11T01:28:08Z

👎 Rejected by code reviews

mohamed82008 · 2020-08-11T01:28:53Z

bors r+

bors · 2020-08-11T01:28:55Z

👎 Rejected by code reviews

mohamed82008 · 2020-08-11T01:29:20Z

@torfjelde It seems that your requested changes are blocking bors 😄

mohamed82008 · 2020-08-11T01:40:59Z

bors try

bors · 2020-08-11T01:47:15Z

try

Build failed:

test (1.0, ubuntu-latest, x64)

mohamed82008 · 2020-08-11T01:50:49Z

There seem to be issues with Bijectors https://github.com/TuringLang/DynamicPPL.jl/runs/969319108#step:5:58.

torfjelde

To satisfy bors!

torfjelde · 2020-08-11T13:22:23Z

Btw, bors is probably complaining for the same reason as this: TuringLang/Bijectors.jl#126 (comment)

yebai · 2020-08-11T14:46:58Z

Maybe merge this one manually as an exception since the falling test is from Bijectors?

mohamed82008 · 2020-08-11T14:48:26Z

Thanks @devmotion!

devmotion added 2 commits June 28, 2020 02:11

Fix prob macro (issues #137 and #138)

b6df51f

Add tests

5621f7e

mohamed82008 reviewed Jun 29, 2020

View reviewed changes

Merge branch 'dev' into probmacro_bugfixes

0f27ae7

bors bot added a commit that referenced this pull request Jul 3, 2020

Try #147:

c949028

bors bot added a commit that referenced this pull request Jul 3, 2020

Try #147:

0983b1e

devmotion mentioned this pull request Jul 10, 2020

Extracting sampled parameters is broken TuringLang/Turing.jl#1353

Closed

yebai approved these changes Jul 20, 2020

View reviewed changes

bors bot pushed a commit that referenced this pull request Jul 20, 2020

Fix prob macros (#147)

8daa016

This PR fixes #137 and #138, and allows to update a `VarInfo` object with a named tuple. Co-authored-by: David Widmann <[email protected]>

This was referenced Jul 27, 2020

Makes _setval! and thus the prob-macro compatible with v4 #151

Closed

Dot-tilde (.~) broadcasting not supported in Turing models TuringLang/Turing.jl#1360

Closed

torfjelde requested changes Jul 28, 2020

View reviewed changes

Apply suggestions from code review

7a64232

Co-authored-by: Tor Erlend Fjelde <[email protected]>

torfjelde mentioned this pull request Aug 11, 2020

Problems with deterministic distribution TuringLang/Turing.jl#1335

Closed

mohamed82008 approved these changes Aug 11, 2020

View reviewed changes

mohamed82008 closed this Aug 11, 2020

mohamed82008 reopened this Aug 11, 2020

bors bot added a commit that referenced this pull request Aug 11, 2020

Try #147:

63543dd

torfjelde approved these changes Aug 11, 2020

View reviewed changes

yebai approved these changes Aug 11, 2020

View reviewed changes

mohamed82008 merged commit 5d72768 into dev Aug 11, 2020

bors bot deleted the probmacro_bugfixes branch August 11, 2020 14:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix prob macros #147

Fix prob macros #147

devmotion commented Jun 28, 2020

mohamed82008 Jun 29, 2020

mohamed82008 Jun 29, 2020

devmotion Jun 29, 2020

yebai Jul 20, 2020

torfjelde Jul 28, 2020

devmotion commented Jul 3, 2020

bors bot commented Jul 3, 2020

devmotion commented Jul 3, 2020

bors bot commented Jul 3, 2020

yebai commented Jul 20, 2020

bors bot commented Jul 20, 2020

yebai commented Jul 20, 2020

bors bot commented Jul 20, 2020

yebai commented Jul 20, 2020

torfjelde left a comment

torfjelde Jul 28, 2020

torfjelde Jul 28, 2020

devmotion Jul 28, 2020

torfjelde Jul 29, 2020

devmotion Jul 29, 2020

torfjelde commented Aug 11, 2020

mohamed82008 commented Aug 11, 2020

bors bot commented Aug 11, 2020

mohamed82008 commented Aug 11, 2020

bors bot commented Aug 11, 2020

mohamed82008 commented Aug 11, 2020 •

edited

Loading

mohamed82008 commented Aug 11, 2020

bors bot commented Aug 11, 2020

mohamed82008 commented Aug 11, 2020

torfjelde left a comment

torfjelde commented Aug 11, 2020

yebai commented Aug 11, 2020

mohamed82008 commented Aug 11, 2020

	"""
	TypedVarInfo(vi::UntypedVarInfo)

	This function finds all the unique `sym`s from the instances of `VarName{sym}` found in
	`vi.metadata.vns`. It then extracts the metadata associated with each symbol from the
	global `vi.metadata` field. Finally, a new `VarInfo` is created with a new `metadata` as
	a `NamedTuple` mapping from symbols to type-stable `Metadata` instances, one for each
	symbol.
	"""

Fix prob macros #147

Fix prob macros #147

Conversation

devmotion commented Jun 28, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

devmotion commented Jul 3, 2020

bors bot commented Jul 3, 2020

try

devmotion commented Jul 3, 2020

bors bot commented Jul 3, 2020

try

yebai commented Jul 20, 2020

bors bot commented Jul 20, 2020

yebai commented Jul 20, 2020

bors bot commented Jul 20, 2020

yebai commented Jul 20, 2020

torfjelde left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

torfjelde commented Aug 11, 2020

mohamed82008 commented Aug 11, 2020

bors bot commented Aug 11, 2020

mohamed82008 commented Aug 11, 2020

bors bot commented Aug 11, 2020

mohamed82008 commented Aug 11, 2020 • edited Loading

mohamed82008 commented Aug 11, 2020

bors bot commented Aug 11, 2020

try

mohamed82008 commented Aug 11, 2020

torfjelde left a comment

Choose a reason for hiding this comment

torfjelde commented Aug 11, 2020

yebai commented Aug 11, 2020

mohamed82008 commented Aug 11, 2020

mohamed82008 commented Aug 11, 2020 •

edited

Loading