-
Notifications
You must be signed in to change notification settings - Fork 37
VarNamedTuple, with an application for FastLDF #1150
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
Benchmark Report
Computer InformationBenchmark Results |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## breaking #1150 +/- ##
============================================
- Coverage 81.67% 78.10% -3.58%
============================================
Files 42 43 +1
Lines 3930 4165 +235
============================================
+ Hits 3210 3253 +43
- Misses 720 912 +192 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
It looks to me that the 1.11 perf is only a lot worse on the trivial model. In my experience (ran into this exact issue with Enzyme once, see also https://github.com/TuringLang/DynamicPPL.jl/pull/877/files), trivial models with 1 variable can be quite susceptible to changes in inlining strategy. It may be that a judicious |
I decided that rather than take over VarInfo like in #1074, the first use case of VarNamedTuple should be replacing the NamedTuple/Dict combo in FastLDF. That's what this PR does.
This is still work in progress:
Colons inVarNames.However, tests seem to pass, so I'm putting this up. I ran the familiar FastLDF benchmarks from #1132, adapted a bit. Source code:
Results on Julia v1.12:
Same thing but in Julia v1.11:
So on 1.12 all looks good: This is a bit faster than the old version, substantial faster when there are a lot of IndexLenses, as it should. On 1.11 performance is destroyed, probably because type inference fails/gives up, and I need to fix that.
The main point of this PR is not performance, but having a general data structure for storing information keyed by VarNames, so I'm happy as long as performance doesn't degrade. Next up would be using this same data structure for ConditionContext (hoping to fix #1148), ValuesAsInModelAcc, maybe some other Accumulators, InitFromParams, GibbsContext, and finally to implement an AbstractVarInfo type.
I'll update the docs page with more information about what the current design is that I've implemented, but the one sentence summary is that it's nested NamedTuples, and then whenever we meet IndexLenses, it's an Array for the values together with a mask-Array that marks which values are valid values and which are just placeholders.
I think I know how to fix all the current short-comings, except for
Colons inVarNames. Setting a value in a VNT with aColoncould be done, but getting seems ill-defined, at least without providing further information about the size the value should be.cc @penelopeysm, though this isn't ready for reviews yet.