Conversation
Benchmark Report
Computer InformationBenchmark Results |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## breaking #1327 +/- ##
============================================
+ Coverage 78.19% 78.64% +0.44%
============================================
Files 50 50
Lines 3582 3521 -61
============================================
- Hits 2801 2769 -32
+ Misses 781 752 -29 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
DynamicPPL.jl documentation for PR #1327 is available at: |
461b5c9 to
d2585a3
Compare
|
@sunxd3 would you be able to take a look? Here's the TLDR and my overall thoughts on this:
|
sunxd3
left a comment
There was a problem hiding this comment.
I think the type hierarchy changes all makes sense. Although it is very breaking, even though I don't imagine a lot of people depend on the transformation interface, it might still worth giving some deprecation buffer (we can talk about this in the meeting).
The general interface also make sense to me.
Multivariate 10k has some performance regression on Mooncake, I wonder why.
| supposed to be *fixed*, i.e., they should not depend on random choices made during model | ||
| execution! | ||
| """ | ||
| function get_fixed_transforms( |
There was a problem hiding this comment.
an idea: this function can be run n times and compare the if the results are the same
| @doc """ | ||
| getindex(vi::AbstractVarInfo, vn::VarName[, dist::Distribution]) | ||
| getindex(vi::AbstractVarInfo, vns::Vector{<:VarName}[, dist::Distribution]) | ||
|
|
||
| Return the current value(s) of `vn` (`vns`) in `vi` in the support of its (their) | ||
| distribution(s). | ||
|
|
||
| If `dist` is specified, the value(s) will be massaged into the representation expected by `dist`. | ||
| """ Base.getindex |
| vectorised values in `vnt` to have the corresponding transforms from `transforms_vnt`. | ||
|
|
||
| This function returns a VarNamedTuple mapping all VarNames to their corresponding | ||
| `RangeAndTransform`. |
There was a problem hiding this comment.
need update?
| `RangeAndTransform`. | |
| `TransformedValue`. |
?
| Note that preparing a `LogDensityFunction` with an AD type `AutoBackend()` requires the AD | ||
| backend itself to have been loaded (e.g. with `import Backend`). | ||
|
|
||
| Finally, the `fix_transform` keyword argument allows you to specify whether the transforms |
There was a problem hiding this comment.
| Finally, the `fix_transform` keyword argument allows you to specify whether the transforms | |
| Finally, the `fix_transforms` keyword argument allows you to specify whether the transforms |
|
The biggest break from this might be the removal of |
Closes #1249. I'll write more here later, but you can see the changelog for a good overview of this PR.
Things to do
Possibly in a separate PR:
bijector(model), and whether that can be removed (though, note also VI interface is too low-level Turing.jl#2783)Some scripts to use to benchmark: