link and invlink should correctly work with Selector and thus Gibbs
#542
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
It's worth noting that once we have TuringLang/Turing.jl#2099 all this shenanigans with |
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
|
This now looks good to go @devmotion @yebai ! |
There was a problem hiding this comment.
Thanks, @torfjelde -- I have a few clarity questions below. Otherwise, this PR looks good to me.
| ) where {names,space} | ||
| vals = Expr(:tuple) | ||
| for f in names | ||
| if inspace(f, space) || length(space) == 0 |
There was a problem hiding this comment.
| if inspace(f, space) || length(space) == 0 | |
| # we select all variables in `varinfo` if `space = nothing`, | |
| if inspace(f, space) || length(space) == 0 |
There was a problem hiding this comment.
space is never nothing though. At "best" it's an empty tuple. Remember, space !== vns. The scenario with vns === nothing only comes into play in the next call.
| vns = metadata.vns | ||
|
|
||
| # Construct the new transformed values, and keep track of their lengths. | ||
| vals_new = map(vns) do vn | ||
| # Return early if we're already in unconstrained space. | ||
| if istrans(varinfo, vn) | ||
| # HACK: if `target_vns` is `nothing`, we ignore the `target_vns` check. |
There was a problem hiding this comment.
It's not entirely clear to me why this is a HACK.
There was a problem hiding this comment.
Maybe HACK isn't quite the right word, but it's somewhat ugly design IMO. And the overall thing of "defining a new _getvns which does something slightly different only for a particular sampler" is hack.
Co-authored-by: Hong Ge <[email protected]>
Pull Request Test Coverage Report for Build 6463168354
💛 - Coveralls |
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #542 +/- ##
==========================================
+ Coverage 81.39% 81.51% +0.12%
==========================================
Files 25 25
Lines 2983 3003 +20
==========================================
+ Hits 2428 2448 +20
Misses 555 555
☔ View full report in Codecov by Sentry. |
TuringLang/Turing.jl#2096 is failing because the current implementations of
linkandinvlinkignore the "space" of the sampler.This PR makes it so that
linkandinvlinknow more closely replicates the behavior oflink!!andinvlink!!.I saw "more closely" because they are still different. The "immutable" versions supports changing the sizes of the underlying storage (which is necessary when we're working with distribution whose dimensionality changes when linking, e.g.
Dirichlet).