[Merged by Bors] - Linearization/flattening of SimpleVarInfo#417
Closed
torfjelde wants to merge 245 commits into
Closed
[Merged by Bors] - Linearization/flattening of SimpleVarInfo#417torfjelde wants to merge 245 commits into
torfjelde wants to merge 245 commits into
Conversation
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: David Widmann <devmotion@users.noreply.github.com>
…PPL.jl into tor/link-improvements
yebai
approved these changes
Nov 2, 2022
Member
There was a problem hiding this comment.
Many thanks, @torfjelde -- I reviewed this PR carefully. It is very readable now. I think you did a nice job by introducing a set of APIs for AbstractVarInfo. Please see below for some comments. Among them, the major ones are:
- Consider renaming
MaybeThreadSafeVarInfotoVarInfoOrThreadSafeVarInfo - Consider avoiding using
Bijector.logpdf_with_transso that we can finish #342 later.
This was referenced Nov 2, 2022
Member
A bit late, but one can check the dashboard here if it is unclear if/what the problem is: https://app.bors.tech/repositories/24589 (one can navigate to it from the bors website or, IIRC, from the bors checks on Github) One can cancel borg with |
Member
Author
Sounds good!
IMO this should be a separate PR. Right now, the focus is just on making it so that we can actually start using And thank you for the information @devmotion ! Very helpful:) |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
yebai
approved these changes
Nov 3, 2022
Member
|
bors try |
Contributor
tryAlready running a review |
Member
|
bors try |
Member
|
Deleting the |
Contributor
Member
Author
|
bors r+ |
bors Bot
pushed a commit
that referenced
this pull request
Nov 4, 2022
This PR introduces a couple of things, though these are related:
1. `unflatten(original[, spl], x)`: converts from a certain input `x`, usually a `Vector`, into an instance similar to `original`.
- Effectively the same as the current constructor `VarInfo(varinfo_old, spl, x)`.
- I looked into using ParameterHandling.jl for this but decided against it for a couple of reasons:
- Seems overkill.
- `unflatten`-equivalent is constructed as a closure, which means that we need to keep track of this returned method rather than just using a "template" `AbstractVarInfo` + construction of unflattening requires construction of the flatten representation + the way one specifies the types is a bit too opinionated (which causes some issues with certain AD-frameworks) + closures can have less desirable performance characteristics.
- The current Turing.jl-codebase is easily adapted to this `unflatten` since it's really just a matter of replacing calls `VarInfo(varinfo_old, spl, x)` with `unflatten(varinfo_old, spl, x)`. A ParameterHandling.jl approach will require more work.
2. `link!!` and `invlink!!`, BangBang-versions of `link!` and `invlink!`, respectively, with some differences:
- These take additional arguments which should always be sufficient to determine the transformation. These are:
- `model`
- `sampler`
- `t::AbstractTransformation`. This sets us up for allowing alternative transformations to be used. As of right now, this only has an affect when calling `link!!` and `invlink!!`, _not_ when used inside of the tilde-pipeline.
- Also adds the logabsdet-jacobian term to the `logp`, so that `getlogp(vi) ≠ getlogp(link!!(vi))` holds. This allows us to compute, say, `logjoint` by _first_ linking `vi` in a single pass, and then compute `logjoint(settrans!(vi, NoTransformation()), θ_constrained)`. Such a pattern, in particular if the transformation has been specified by the user themselves, will usually have much better performance than the `logpdf_with_trans(..., true)` within the tilde-callstack.
3. `make_default_varinfo(rng, model, sampler)` which allows one to overload on a, say, per-model or model-sampler-combination basis to specify which implementation of `AbstractVarInfo` to use.
- Not entirely happy with this approach 😕
EDIT: This should be merged _after_ #420
Co-authored-by: Hong Ge <hg344@cam.ac.uk>
Contributor
|
Pull request successfully merged into master. Build succeeded: |
bors
Bot
changed the title
alyst
pushed a commit
to alyst/DynamicPPL.jl
that referenced
this pull request
Mar 21, 2023
This PR introduces a couple of things, though these are related:
1. `unflatten(original[, spl], x)`: converts from a certain input `x`, usually a `Vector`, into an instance similar to `original`.
- Effectively the same as the current constructor `VarInfo(varinfo_old, spl, x)`.
- I looked into using ParameterHandling.jl for this but decided against it for a couple of reasons:
- Seems overkill.
- `unflatten`-equivalent is constructed as a closure, which means that we need to keep track of this returned method rather than just using a "template" `AbstractVarInfo` + construction of unflattening requires construction of the flatten representation + the way one specifies the types is a bit too opinionated (which causes some issues with certain AD-frameworks) + closures can have less desirable performance characteristics.
- The current Turing.jl-codebase is easily adapted to this `unflatten` since it's really just a matter of replacing calls `VarInfo(varinfo_old, spl, x)` with `unflatten(varinfo_old, spl, x)`. A ParameterHandling.jl approach will require more work.
2. `link!!` and `invlink!!`, BangBang-versions of `link!` and `invlink!`, respectively, with some differences:
- These take additional arguments which should always be sufficient to determine the transformation. These are:
- `model`
- `sampler`
- `t::AbstractTransformation`. This sets us up for allowing alternative transformations to be used. As of right now, this only has an affect when calling `link!!` and `invlink!!`, _not_ when used inside of the tilde-pipeline.
- Also adds the logabsdet-jacobian term to the `logp`, so that `getlogp(vi) ≠ getlogp(link!!(vi))` holds. This allows us to compute, say, `logjoint` by _first_ linking `vi` in a single pass, and then compute `logjoint(settrans!(vi, NoTransformation()), θ_constrained)`. Such a pattern, in particular if the transformation has been specified by the user themselves, will usually have much better performance than the `logpdf_with_trans(..., true)` within the tilde-callstack.
3. `make_default_varinfo(rng, model, sampler)` which allows one to overload on a, say, per-model or model-sampler-combination basis to specify which implementation of `AbstractVarInfo` to use.
- Not entirely happy with this approach 😕
EDIT: This should be merged _after_ TuringLang#420
Co-authored-by: Hong Ge <hg344@cam.ac.uk>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR introduces a couple of things, though these are related:
unflatten(original[, spl], x): converts from a certain inputx, usually aVector, into an instance similar tooriginal.VarInfo(varinfo_old, spl, x).unflatten-equivalent is constructed as a closure, which means that we need to keep track of this returned method rather than just using a "template"AbstractVarInfo+ construction of unflattening requires construction of the flatten representation + the way one specifies the types is a bit too opinionated (which causes some issues with certain AD-frameworks) + closures can have less desirable performance characteristics.unflattensince it's really just a matter of replacing callsVarInfo(varinfo_old, spl, x)withunflatten(varinfo_old, spl, x). A ParameterHandling.jl approach will require more work.link!!andinvlink!!, BangBang-versions oflink!andinvlink!, respectively, with some differences:modelsamplert::AbstractTransformation. This sets us up for allowing alternative transformations to be used. As of right now, this only has an affect when callinglink!!andinvlink!!, not when used inside of the tilde-pipeline.logp, so thatgetlogp(vi) ≠ getlogp(link!!(vi))holds. This allows us to compute, say,logjointby first linkingviin a single pass, and then computelogjoint(settrans!(vi, NoTransformation()), θ_constrained). Such a pattern, in particular if the transformation has been specified by the user themselves, will usually have much better performance than thelogpdf_with_trans(..., true)within the tilde-callstack.make_default_varinfo(rng, model, sampler)which allows one to overload on a, say, per-model or model-sampler-combination basis to specify which implementation ofAbstractVarInfoto use.EDIT: This should be merged after #420