-
Notifications
You must be signed in to change notification settings - Fork 37
Make FastLDF the default + remove SimpleVarInfo #1139
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
base: breaking
Are you sure you want to change the base?
Conversation
79a440c to
64ad4af
Compare
Benchmark Report
Computer InformationBenchmark Results |
|
DynamicPPL.jl documentation for PR #1139 is available at: |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## breaking #1139 +/- ##
============================================
- Coverage 81.60% 77.21% -4.40%
============================================
Files 42 40 -2
Lines 3925 3708 -217
============================================
- Hits 3203 2863 -340
- Misses 722 845 +123 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
3e646ca to
0dd2f70
Compare
c4f93fe to
c1ee6cc
Compare
0dd2f70 to
077457f
Compare
d86ff22 to
be27184
Compare
0150ef4 to
2fad97b
Compare
0cc9278 to
6f5df1a
Compare
177656b to
9310ec0
Compare
|
@mhauru Feel free to not do a full review at this stage, right now I'm more curious as to your thoughts about SVI. Happy to either keep it in for now, or remove it outright. Regardless of which way we go, I can rebase this against breaking so that this can actually be reviewed. |
9310ec0 to
6a52955
Compare
|
Trying to think of reasons to keep SVI, and the only one I can think of would be if we, or someone else, used it with one of the methods that don't use LDF. It hasn't traditionally worked with particle samplers because it didn't have Its days are definitely numbered. The only reason I can think of for not doing it now would be to investigate using it with particle samplers, but I doubt we'll get to that. So I'd lean towards getting the code simplification straight-away, make it easier to work with VarInfo as we keep refactoring. |
6a52955 to
992cea9
Compare
7fa0986 to
6849bc2
Compare
|
I think CloudFlare is responsible for the remaining ordinary test suite failures. The perf benchmarks are pretty much where I expected them to be.
The gradient evaluation benchmarks are not amazing, but I think it's mainly because any perf gains for these models are in primal evaluation, and so if the gradient doesn't also get sped up, it looks bad. Arguably there should be a third column which is |
6849bc2 to
b1368dd
Compare
|
|
||
| #### SimpleVarInfo | ||
|
|
||
| `SimpleVarInfo` has been removed. |
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.
SimpleVarInfo has cleaner code than VarInfo. Doesn't it make more sense to remove VarInfo and replace it with SimpleVarInfo?
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.
Markus can probably say more, but I don't think the choice is between SVI and VI. The structs are actually the same except that SVI carries an extra field to store a transformation (which would typically be part of VI's metadata field).
DynamicPPL.jl/src/simple_varinfo.jl
Lines 188 to 196 in 052bc19
| struct SimpleVarInfo{NT,Accs<:AccumulatorTuple where {N},C<:AbstractTransformation} <: | |
| AbstractVarInfo | |
| "underlying representation of the realization represented" | |
| values::NT | |
| "tuple of accumulators for things like log prior and log likelihood" | |
| accs::Accs | |
| "represents whether it assumes variables to be transformed" | |
| transformation::C | |
| end |
Lines 106 to 109 in 052bc19
| struct VarInfo{Tmeta,Accs<:AccumulatorTuple} <: AbstractVarInfo | |
| metadata::Tmeta | |
| accs::Accs | |
| end |
I think the complexity lies in what kinds of metadata they carry. SVI code looks simpler because src/simple_varinfo.jl only handles NamedTuple or Dict metadata. VarInfo code looks terrible because src/varinfo.jl mostly deals with Metadata metadata (hence all the generated functions).
So instead of saying removing SVI, maybe it would have been more accurate to say that this is removing NamedTuple and Dict as supported forms of metadata. As far as I can tell it's mostly a coincidence that this code is associated with SVI.
If the aim is to clean up src/varinfo.jl, then what really needs to happen is to replace Metadata with VNV (although VNV has 1.7k lines of code to itself, so IMO that's not exactly clean; but it probably feels better because it's in its own file, more documented, etc.)
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.
(#1105)
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.
IIRC, the plan is to use SimpleVarInfo with VNV and VNT as the unifying tracing data type, largely because SimpleVarInfo has a cleaner API and better documentation. I agree that the differences between SVI and VI aren’t all that significant.
There are two commits in this PR:
Replace
LogDensityFunctionwithFastLDF(that's not controversial);Remove
SimpleVarInfo(which might be controversial).SVI
I would argue that the main purpose of SVI was for performance when evaluating a model with NamedTuple or Dict parameters:
However, exactly the same thing can be accomplished now with the combination of
InitFromParams+OnlyAccsVarInfo:Indeed, this strategy is on par with SVI for the NamedTuple case and faster than SVI for the Dict case (see benchmarks on #1125; performance characteristics on this PR are the same).
The other context in which SVI was useful was evaluation with vector parameters, but that's handled by FastLDF. Thus, my conclusion is that SVI is no longer needed.
Furthermore, because SVI is gone, I believe
DynamicTransformation,StaticTransformation, andmaybe_invlink_before_eval!!can also go. However, we'll leave that for another time.I looked through GitHub for usage of SimpleVarInfo, and it's only in DPPL, Turing, Mooncake benchmarks (trivially fixed once FastLDF is released), and https://github.com/epimap/Epimap.jl-public and https://github.com/marius311/MuseInference.jl, which haven't been updated in a while.
Miscellany
I'm lumping these into a single PR, but also happy to split them up. The two commits are completely standalone though. The main purpose of lumping them is so that I have a single branch to test Turing CI against.