-
Notifications
You must be signed in to change notification settings - Fork 36
ProductNamedTupleDistribution compatibility #1079
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
Conversation
DynamicPPL.jl documentation for PR #1079 is available at: |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## breaking #1079 +/- ##
============================================
- Coverage 81.13% 81.06% -0.08%
============================================
Files 40 40
Lines 3722 3749 +27
============================================
+ Hits 3020 3039 +19
- Misses 702 710 +8 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Here are some docs, to be used in a future PR:
|
CI failures are unrelated, I'll Edit: #1081 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work. I agree that all the from/to_
transform stuff gets confusing.
Bonus: the file is editable in nvim again |
struct ProductNamedTupleUnvecTransform{names,T<:NamedTuple{names}} | ||
dists::T | ||
# The `i`-th input range corresponds to the segment of the input vector | ||
# that belongs to the `i`-th distribution. | ||
input_ranges::Vector{UnitRange} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a subpar data structure because it assumes that dists and input_ranges have the same length (this is enforced in the inner constructor). I think in an ideal world we would combine dists and input_ranges into a single NamedTuple ... but the issue with that is that to make it type stable I think we'd have to make the constructor also a generated function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very nice.
Co-authored-by: Markus Hauru <[email protected]>
854a6cb
to
3f31d4b
Compare
Closes #1077
Closes TuringLang/Turing.jl#2659
This PR implements the necessary methods to get ProductNamedTupleDistribution working in Turing. I've tested with this PR + TuringLang/Turing.jl#2689 and this model works with both MH (i.e. unlinked) and NUTS (i.e. linked). It contains the absolute most nightmare scenario where there is a nested
PNTDist
plusLKJCholesky
inside aPNTDist
.This is all type stable too:
This PR also adds tests for all the transform methods for a number of typical distributions.
future work
I think these should be a separate PR, but:
from_vec_transform
stuff. I spent way too long trying to figure out the meaning of things. It's IMO not clear what the inputs and outputs are expected to be, and some examples would go a long way.from_vec_transform
but I didn't have to implementto_vec_transform
because the Metadata/VNV code just skips over it and goes straight totovec
. I think Metadata/VNV code should useto_vec_transform
and the default definition ofto_vec_transform
should internally use_tovec
.from_vec_transform(dist::Distribution)
but in particular when handling VarNamedVector it also usesfrom_vec_transform(x)
wherex
is a sample drawn fromdist
. I think the latter should be renamed.