-
Notifications
You must be signed in to change notification settings - Fork 37
VNT Part 5: VarNamedTuple as VarInfo #1183
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: mhauru/vnt-for-fastldf
Are you sure you want to change the base?
Conversation
Benchmark Report
Computer InformationBenchmark Results |
|
Darn, that is really good.
Am I right in saying the latter two are only really needed for DefaultContext? Edit: Actually, that's a silly question, if not for DefaultContext we don't even need the metadata field at all. |
|
Also, I'm just eyeing this PR and thinking that it's a prime opportunity to clean up the varinfo interface, especially with the functions that return internal values when they probably shouldn't. |
Yes. I'm first trying to make this work without making huge interface changes, just to make sure this can do everything that is needed to do, but I think interface changes should follow close behind, maybe in the same PR or the same release. They'll be much easier to make once there is only two VarInfo types that need to respect them, namely the new one and Threadsafe. |
|
Looks exciting! Two quick quesitons: would this be suitable to
|
One thing I haven't benchmarked, and maybe should, is type unstable models. There is a possibility that type unstable models will be slower with the new approach, because |
| @@ -0,0 +1,247 @@ | |||
| struct VNTVarInfo{T<:VarNamedTuple,Accs<:AccumulatorTuple} <: AbstractVarInfo | |||
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.
It's okay to just call this VarInfo -- all old VarInfo / SimpleVarInfo code should be deprecated and deleted.
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.
Would you prefer Trace over VarInfo? I think you suggested it on Slack last time and I would be on board with that.
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.
It's okay to just call this VarInfo -- all old VarInfo / SimpleVarInfo code should be deprecated and deleted.
I agree. I called it VNTVarInfo while the old ones are still around (I want to compare against them), but by the time this PR is ready for review the others should be gone and VNTVarInfo should be just VarInfo.
I'm happy to rename, but would leave it for a different PR. Maybe the right time to rename is when Accumulators are separated out from VarInfo?
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.
To clarify, do we plan to split VarInfo into two parts, eg, Accumulator and VarNamedTuple?
It's okay to have a a type to keep these two together.
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.
Would you prefer Trace over VarInfo? I think you suggested it on Slack last time and I would be on board with that.
Trace is more aligned with the literature. Keeping the VarInfo name would stay close to our Turing.jl ACM paper. I am okay with both.
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.
I think we are almost at a point where there's no need to keep them together.
|
One comment here is to carefully consider the requirements of particle Gibbs during reviewing so we have sufficient design prevision for
|
|
I think PG should be fine. I think we aren't really removing things so much as shuffling things around and putting them in the right boxes -- so previously where Libtask had to carry a full varinfo, we would now just make it carry a VNT + accumulator tuple. |
I put together a quick sketch of what it would look like to use
VarNamedTupleas aVarInfodirectly. By that I mean having aVarInfotype that is nothing but accumulators plus aVarNamedTuplethat maps eachVarNameto a tuple (or actually a tiny struct, but anyway) of three values: Stored value for this variable, whether it's linked, and what transform should be applied to convert the stored value back to "model space". I'm calling this new VarInfo typeVNTVarInfo(name to be changed later).This isn't finished yet, but the majority of tests pass. There are a lot of failures around edge cases like Cholesky and weird VarNames and such, but for most simple models you can do
and it'll give you the correct result.
unflattenandvi[:]also work.I'll keep working on this, but at this point I wanted to pause to do some benchmarks, see how viable this is. Benchmark code, very similar to #1182, running
evaluate!!on our benchmarking models:Details
Results contrasting the new
VarInfowith both the oldVarInfoand withSimpleVarInfo{NamedTuple}andSimpleVarInfo{OrderedDict}. Some SVI NT results are missing because it couldn't handle theIndexLenses:I think a fair TL;DR is that for both small models and models with
IndexLenses this is many times faster than the oldVarInfo, and not far off fromSimpleVarInfowhenSimpleVarInfois at its fastest (NamedTuples for small models, OrderedDicts forIndexLenses). I would still like to close that gap a bit, I don't know why linking causes such a large slowdown in some cases, I suspect it's because the transform system is geared towards assuming we want to vectorise things, and I've hacked this together quickly to just get it to work.For large models performance is essentially equal, as it should be, because this is about overheads. To fix that, I need to look into using views in some clever way, but that's for later.
I think this is a promising start towards being able to say that all of
VarInfo,SimpleVarInfo, andVarNamedVectorcould be replaced with a direct use ofVarNamedTuple(as opposed to e.g. VNT wrappingVarNamedVector), and it would be pretty close to being a best-of-all-worlds solution, in that it's almost as fast as SVI and has full support for all models.Note that the new VNTVarInfo has no notion of typed and untyped VarInfos. They are all as typed as they can be, which should also help simplify code.
I'll keep working on this tomorrow.