Depreciate@submodel l ~ m in favour of l ~ to_submodel(m); rename generated_quantities to returned

[torfjelde]

This adds the @returned_quantities macro as discussed @yebai @mhauru

This is meant to be a replacement for @submodel macro, but without the ability to do automatic prefixing. It ends up looking like

julia> @model function demo1(x)
           x ~ Normal()
           return 1 + abs(x)
       end;

julia> @model function demo2(x, y, z)
            a ~ to_submode(prefix(demo1(x), "sub1"))
            b ~ to_submodel(prefix(demo1(y), "sub2"))
            return z ~ Uniform(-a, b)
       end;

julia> rand(demo2(missing, missing, 0.4))
(var"sub1.x" = 0.5865756059371534, var"sub2.x" = -0.25563799658500047)

Likely TODOs:

• Add deprecation warning to @submodel telling the user to use @returned_quantities.
• Do we do the renaming of generated_quantities to returned_quantities in this PR?

Fix #691

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 86.48%. Comparing base (82842bc) to head (b467c75).
Report is 1 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #696      +/-   ##
==========================================
+ Coverage   86.38%   86.48%   +0.09%     
==========================================
  Files          35       35              
  Lines        4180     4209      +29     
==========================================
+ Hits         3611     3640      +29     
  Misses        569      569              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[coveralls]

Pull Request Test Coverage Report for Build 12144617184

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

• For more information on this, see Tracking coverage changes with pull request builds.
• To avoid this issue with future PRs, see these Recommended CI Configurations.
• For a quick fix, rebase this PR at GitHub. Your next report should be accurate.

Details

• 34 of 36 (94.44%) changed or added relevant lines in 6 files are covered.
• 30 unchanged lines in 11 files lost coverage.
• Overall coverage increased (+0.09%) to 86.481%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
src/model.jl	14	16	87.5%

Files with Coverage Reduction	New Missed Lines	%
src/simple_varinfo.jl	1	86.08%
src/varnamedvector.jl	1	89.42%
src/sampler.jl	1	94.55%
src/debug_utils.jl	1	94.39%
ext/DynamicPPLEnzymeCoreExt.jl	1	0.0%
ext/DynamicPPLForwardDiffExt.jl	2	72.22%
src/utils.jl	3	83.16%
src/distribution_wrappers.jl	4	52.78%
src/contexts.jl	4	76.38%
src/values_as_in_model.jl	4	63.64%

Totals
Change from base Build 12138508432:	0.09%
Covered Lines:	3640
Relevant Lines:	4209

---

💛 - Coveralls

[yebai]

@torfjelde I suggest we change the prefix feature to a prefix_variables model operation (feel free to come up with better names). Then we could use the same functionality prefix_variables in more places, e.g.

# submodel prefixing
julia> @model function demo2(x, y, z)
            a = @returned_quantities prefix_variables(demo1(x), "sub1")
            b = @returned_quantities prefix_variables(demo1(y), "sub2")
            return z ~ Uniform(-a, b)
       end;

julia> rand(demo2(missing, missing, 0.4))
(var"sub1.x" = 0.5865756059371534, var"sub2.x" = -0.25563799658500047)

# rand prefixing 

julia> ret = rand(prefix_variables(demo1(1.), "prior_sample"))

# generated quantities / predict 

julia> returned_quantities(prefix_variables(demo1(1.), "generated_var_"), chain) 

This would also help unify the syntax of @generated_qunatities and generated_quantities- IIRC, the only difference between them is that generated_quantities lacks the prefixing/renaming feature.

This could be further unified with NamedDist in the future. See, e.g., #414

[torfjelde]

We already have DynamicPPL.prefix, though this doesn't do exactly what you want here. We could easily just add

prefix(model::Model, x) = contextualize(model, PrefixContext(model.context, Symbol(x)))

or something as an additional definition.

However, I'm a bit worred about

1. It's quite verbose + a bit "too close to internals" for end-users.
2. To achieve the same performance guarantees that we have currently, we need to wrap everything in Val before calling prefix(model, ...) 😕 This seems non-ideal to me vs. the current approach.

[yebai]

It's quite verbose + a bit "too close to internals" for end-users.

I like the @returned_quantities(prefix(model, "prefix_")) syntax because it is

• less mysterious than @returned_quantities model "prefix_"
• all the other model operations could share this, e.g. rand(prefix(model, "prefix_")) to verify the effects of prefixing, which is very useful

prefix(model, x) is NOT any closer to internals than any other model operation APIs. They are the same, so this is not a problem.

To achieve the same performance guarantees that we have currently, we need to wrap everything in Val before calling prefix(model, ...) 😕 This seems non-ideal to me vs. the current approach.

Point taken, but this is very minor and a bit subjective.

[torfjelde]

Point taken, but this is very minor and a bit subjective.

But this means that the user needs to be careful and do prefix(model, Val{:whatever}()); if we just do prefix(model, :whatever), this will lead to type-instabilities. Do we really want to force end-users of Turing.jl to explicitly use Val? 😕

[yebai]

It is a standard Julia performance trick, so it is okay.

By default, we can print a performance warning message when users call prefix(model, x::String) or similiar.

[yebai]

I'm also happy to turn prefix into a macro: @prefix(model, :prefix_) if that helps. Then we could do

@returned_quantities @prefix(model, :prefix_)

[torfjelde]

Added a @prefix macro:) See the docstring of @returned_quantities for what it looks like 👍

[yebai]

Thanks, @torfjelde; I'm happy with the changes.

To minimise interface confusion (prefix vs. @prefix, and @returned_quantities vs. returned_quantities), shall we consider keeping only @prefix and @returned_quantities and depreciating generated_quantities and prefix?

Thoughts? @mhauru and @penelopeysm

[mhauru]

For prefix/@prefix, maybe keep both but only export the macro? It sounds like unless you know what you are doing, you should use @prefix. And if you know what you're doing, you don't need it be exported. I do generally think it's a good idea to have a macro-free option available if possible.

For returned_quantities/@returned_quantities we still need both, because one is to be used outside of @model, the other inside, right? I forget what we concluded about this in our call, but I do worry users will mix the two up and get confusing errors.

[mhauru]

I found

@returned_quantities prefix=1 + 2 inner()

hard and unintuitive to parse. I think

@returned_quantities prefix=(1 + 2) inner()

would be much clearer. Not sure if this a documentation issue, or if we should disallow the former.

[torfjelde]

That's a documentation issue IMO, as this is not doing any special parsing but reliying on Julia's expression parsing.

[yebai]

For returned_quantities/@returned_quantities we still need both, because one is to be used outside of @model, the other inside, right?

generated_quantities allows users to fix model parameter values and/or accept MCMC chain objects.
We can throw an error if users try to pass fixed parameter values or chain objects to @returned_quantities called within a model.

Then, @returned_quantities can match generated_quantities / returned_quantities exactly, thus allowing us to remove the generated_quantities / returned_quantities altogether.

[torfjelde]

Then, @returned_quantities can match generated_quantities / returned_quantities exactly, thus allowing us to remove the generated_quantities / returned_quantities altogether.

Just so we're all on the same page: @returned_quantities and returned_quantities will not match since the former only takes a single argument, while the other takes two, right? If so, then why would we want to raise explicit errors for incorrect arguments provided vs. just letting Julia raise the "not implemented error"?

[torfjelde]

Deprecated generated_quantities in favour of returned_quantities + removed the prefix=... argument for @prefix.

[torfjelde]

For (2), we could allow theta ~ to_submodel(model_instance, prefix=false) to support your current workflow.

Final question @yebai , in which struct do you want the prefix=false option to live? Sampleable or ReturnedModelWrapper?

EDIT: I put it in Sampleable for now. Let me know if you feel differently.

[yebai]

I put it in Sampleable for now. Let me know if you feel differently.

That looks good to me.

[torfjelde]

Hmm, thought this would be a simple thing to fix (it's just a depwarn happening that I didn't expect to happen). Think there's something funky going on with Documenter.jl...

[torfjelde]

This is now ready to go I think:) Anyone wants a final lookover or just hit the green button?

[mhauru]

I'm happy, just had one question about a docstring. Merge when you consider it ready. Thanks @torfjelde for doing all the heavy lifting here!

[torfjelde]

Seems like we're hitting OOM failures unrelated to this PR again 😕

[yebai]

I merged it manually. This PR has some very nice improvements—many thanks to @torfjelde, @mhauru, and all who helped.