Skip to content

[Merged by Bors] - Fix for #371 #372

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 6 commits into from
Closed

[Merged by Bors] - Fix for #371 #372

wants to merge 6 commits into from

Conversation

torfjelde
Copy link
Member

This PR adds a method called resolve_varnames(varname, dist) and adds an additional generated variable for each ~ which now holds the RHS of ~.

It does address #371 but uncertain if this is the best way, so wouldn't recommend merging this just yet. But putting it here so we can colab on it.

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

@yebai yebai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks sensible to me!

@devmotion
Copy link
Member

uncertain if this is the best way

Another solution would be to remove NamedDist 😛 Maybe someone can remind me, what's the main motivation for NamedDist and what problem does it fix?

@torfjelde
Copy link
Member Author

Another solution would be to remove NamedDist stuck_out_tongue Maybe someone can remind me, what's the main motivation for NamedDist and what problem does it fix?

Allows one to use a different variable-name, e.g. x ~ NamedDist(..., :z) means that this will now show up as z in VarInfo (and the resulting chain, etc.).
Never use it myself, but there are people using it 🤷

@bgroenks96
Copy link
Collaborator

bgroenks96 commented Feb 15, 2022

@devmotion Is there another way besides NamedDist to programmatically define variable names in a Turing model, without using internal APIs like VarInfo?

I am surprised that this is such an uncommon use-case. For large systems with dozens or more parameters, it's a pain to write and maintain Turing models entirely by hand. It's only natural to want to automate this and build models programmatically.

@devmotion
Copy link
Member

Sure, I know this use case, eg from DiffEqBayes. But to me NamedDist seems like a hacky workaround but not a "nice" solution to this problem. Maybe we should just support VarNames on the LHS of ~? Or maybe (an improved version of) the interpolation support is sufficient?

@yebai
Copy link
Member

yebai commented Feb 15, 2022

Maybe we should just support VarNames on the LHS of ~? Or maybe (an improved version of) the interpolation support is sufficient?

It sounds like a good idea!

@yebai
Copy link
Member

yebai commented Feb 15, 2022

For large systems with dozens or more parameters, it's a pain to write and maintain Turing models entirely by hand. It's only natural to want to automate this and build models programmatically.

I’m curious; what models are you working with? Some examples would help us to improve DynamicPPL / AbstractPPL.

@bgroenks96
Copy link
Collaborator

bgroenks96 commented Feb 15, 2022

Maybe we should just support VarNames on the LHS of ~? Or maybe (an improved version of) the interpolation support is sufficient?

That would also be good, maybe even preferable. I actually only learned about NamedDist from @torfjelde , and I was doing something far hackier before via the internal APIs 😅

I’m curious; what models are you working with? Some examples would help us to improve DynamicPPL / AbstractPPL.

I am working on a permafrost model (at the moment basically a 1-D heat PDE with phase change) which uses a kind of "modular" design philosophy; i.e. one builds a model by stacking various components that may include one or more physical processes and each component may have an arbitrary number of parameters associated with it. I am interested in solving the Bayesian inverse problem (and also obtaining predictive uncertainty) for a subset of these parameters.

I actually don't use Turing at the moment for posterior sampling because I found HMC and NUTS to be far too costly for a model with such an expensive likelihood function (I do fairly long simulations of the PDE), and the particle based samplers like SMC in AdvancedPS don't really support proper parallelization. Instead, I use Turing to build and sample from the prior and then I use Ensemble Kalman Sampling for (approximte) posterior inference. In the future, I hope to be able to use Turing for the whole thing :)

I can give you a code snippet of how I specify the Turing model at present:

function makepriors(ps::CryoGridParams)
    return map(ps[:component], ps[:fieldname], ps[:val]) do comp, name, value
        prior(comp, Val{name}(), value)
    end
end
# `NamedDist` comes from DynamicPPL.jl. (this part comes from Tor)
@model priormodel(shared::NamedTuple, name::Symbol, prior::Distribution) = x ~ NamedDist(prior, name)
@model priormodel(shared::NamedTuple, name::Symbol, nt::NamedTuple) = @submodel prefix=$name priormodel(shared, nt)
@model priormodel(shared::NamedTuple, name::Symbol, val::Symbol) = shared[val]
@model priormodel(shared::NamedTuple, name::Symbol, val) = val
@model function priormodel(shared, priors::NamedTuple)
    # NOTE: The below will update `__varinfo__` for every `k`.
    vals = map(keys(priors)) do k
        @submodel priormodel(shared, k, priors[k])
    end
    return NamedTuple{keys(priors)}(vals)
end
@model function cryogridparams(ps::CryoGridParams, sharedpriors::NamedTuple=(;), ::Type{T}=Float64) where T
    shared = @submodel priormodel((;), sharedpriors)
    pvals = @submodel priormodel(shared, (; map(Pair, ps[:pid], ps[:prior])...))
    # create copy of parameter array with compatible type (for autodiff)
    p = copyto!(similar(collect(values(ps)), T), pvals)
    return p
end

where prior is a method that has dispatches for various parameters in the model and CryoGridParams wraps a tabular interface to the parameters provided by ModelParameters.jl.

I hope that helps!

Edit: I should note that I just changed this very recently; the parameters used to be organized into a hierarchy/tree and then priormodel built submodels recursively, hence the priormodel(shared::NamedTuple, name::Symbol, nt::NamedTuple) dispatch. I found this to have some limitations, so now it just uses a table-like structure, and I kind of did the bare minimum to make this code work again... so yeah, that's why it's a bit weird 😅.

@yebai
Copy link
Member

yebai commented Mar 11, 2022

One main advantage of NamedDist is it allows interpolation in VarName at runtime (via an RHS named distribution). TuringLang/AbstractPPL.jl#54 can probably replace this PR since users can create VarNames with interpolation from the LHS.

@phipsgabler can probably confirm or falsify this claim

@phipsgabler
Copy link
Member

phipsgabler commented May 5, 2022

I think VarName interpolation is not sufficient for replacing that. The intent of it was to make manual fiddling with @varname easier, not to have much effect on DPPL. The main work to be done would be in the @model macro, since it has control over interpolation (and we don't use @varname inside of it at all). However, the same kind of logic can maybe be reduce in @model to expand

$name[i]

to

(VarName){name}((Setfield.compose)((Setfield.IndexLens)((i,))))

OK, actually, the LHS is transformed as

$vn = $(AbstractPPL.drop_escape(varname(left)))

so it might actually work... but I haven't tried it!

@yebai
Copy link
Member

yebai commented Jun 8, 2022

@torfjelde a gentle reminder on this

@torfjelde
Copy link
Member Author

As @phipsgabler originally said, @varname interpolation isn't sufficient.

This is because we explicitly "unwrap" the interpolated expressions in the model macro:

# Do not touch interpolated expressions
expr.head === :$ && return expr.args[1]

As a result, even if @varname $name.a works, if you do $name ~ Normal(), the LHS will be parsed as VarName{:name}() and subsequent LHSs in the expanded code will just be name.

If we keep the $ around rather than "unwrapping" it as we do in the lines referenced above, we correctly get VarName{name}() but now subsequent usages of left will be a Expr(:$, name) which aren't interpolated. I've forgotten exactly what's the reason for this, but it probably has something to do with how we escape everything which in turn messes up the macro-hygiene.

@yebai
Copy link
Member

yebai commented Jun 14, 2022

bors try

bors bot added a commit that referenced this pull request Jun 14, 2022
@bgroenks96
Copy link
Collaborator

It would be great to get this merged, if everything is working ok! I currently have my project pointed at this PR branch.

@yebai
Copy link
Member

yebai commented Jun 15, 2022

@torfjelde pls, feel free to go ahead and merge it if you think it is ready.

@torfjelde
Copy link
Member Author

bors r+

@torfjelde
Copy link
Member Author

bors cancel

@bors
Copy link
Contributor

bors bot commented Jun 15, 2022

Canceled.

@torfjelde
Copy link
Member Author

bors r+

bors bot pushed a commit that referenced this pull request Jun 15, 2022
This PR adds a method called `resolve_varnames(varname, dist)` and adds an additional generated variable for each `~` which now holds the RHS of `~`. 

It does address #371 but uncertain if this is the best way, so wouldn't recommend merging this just yet. But putting it here so we can colab on it.

Co-authored-by: Hong Ge <[email protected]>
@bors bors bot changed the title Fix for #371 [Merged by Bors] - Fix for #371 Jun 15, 2022
@bors bors bot closed this Jun 15, 2022
@bors bors bot deleted the tor/fix-371 branch June 15, 2022 14:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants