-
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
Benchmark Report for Commit 3f31d4bComputer InformationBenchmark Results |
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
PNTDistplusLKJCholeskyinside 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_transformstuff. 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_transformbut I didn't have to implementto_vec_transformbecause the Metadata/VNV code just skips over it and goes straight totovec. I think Metadata/VNV code should useto_vec_transformand the default definition ofto_vec_transformshould internally use_tovec.from_vec_transform(dist::Distribution)but in particular when handling VarNamedVector it also usesfrom_vec_transform(x)wherexis a sample drawn fromdist. I think the latter should be renamed.