Implement Symbolic RVs #6072
Merged
Implement Symbolic RVs #6072
Conversation
This file contains 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
ricardoV94
force-pushed
the
symbolic_rv
branch
2 times, most recently
from
a315834 to
d369158
Compare
Codecov Report
@@ Coverage Diff @@
## main #6072 +/- ##
==========================================
- Coverage 91.74% 89.67% -2.07%
==========================================
Files 86 72 -14
Lines 17692 12953 -4739
==========================================
- Hits 16232 11616 -4616
+ Misses 1460 1337 -123
|
ricardoV94
force-pushed
the
symbolic_rv
branch
3 times, most recently
from
825e90b to
be4de60
Compare
lucianopaz
reviewed
Aug 28, 2022
I'll re-request your review then.
Nope. It's all PyMC insfrastructure. |
There was a problem hiding this comment.
This is amazing @ricardoV94! 🙂 I feel like having an explicit SymbolicRandomVariable class would make the code structure of symbolic distributions much more clear.
Would #5931 need to be first merged before this PR?
ricardoV94
force-pushed
the
symbolic_rv
branch
from
be4de60 to
cfd80bb
Compare
ricardoV94
force-pushed
the
symbolic_rv
branch
2 times, most recently
from
7ad9621 to
7fcaa2a
Compare
I don't think so |
ricardoV94
force-pushed
the
symbolic_rv
branch
6 times, most recently
from
bc2c07d to
61a4bb7
Compare
|
The failing |
ricardoV94
force-pushed
the
symbolic_rv
branch
from
61a4bb7 to
754fb97
Compare
junpenglao
approved these changes
Sep 2, 2022
ricardoV94
force-pushed
the
symbolic_rv
branch
from
d4e94a7 to
fe088db
Compare
* This meant that the intended checks were not being run
This allows us to treat these variables as if they were pure RandomVariables (dispatch moments, resize, etc.) Co-authored-by: Michael Osthege <michael.osthege@outlook.com>
* change_rv_size is now called change_dist_size
* Display the dists and their inputs used by SymbolicDistributions
* Also fixes index error in AR when init_dist is scalar and size is None * Remove unused rv_class attribute
This is necessary because the distribution is in fact a "Factory distribution" that needs to be able to resize `sd_dist` dynamically in order to work correctly.
* Implements distribution agnostic univariate RandomWalk
This risk is present when resizing a RandomVariable, whose new size depends on the size of the original RandomVariable. This can lead to wrong update expressions for the reused RNG variables that still depended on the original RandomVariable. Similarly, there is a risk when clone-replacing RandomVariables, as the cloned variables will share the same RNGs. This happened when attempting to use Metropolis with Simulator variables. * change_rv_size does not reuse old RNG * compile_pymc raises if distinct update expression are inferred for the same RNG
ricardoV94
force-pushed
the
symbolic_rv
branch
from
fe088db to
8653d18
Compare
5 tasks
This was referenced Sep 7, 2022
Merged
This was referenced Nov 28, 2022
5 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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 wraps the graphs returned by the old
SymbolicDistribution.rv_opin aSymbolicRVOpFromGraphOpso that they can be manipulated like vanillaRandomVariables. The main benefit is that we can now:SymbolicRVs (nested mixtures, censored mixtures, AR with censored init_dist, you name it)When these
OpFromGraphdon't have a specialized logprob dispatched, they can be inlined inside Aeppl, so that their logprob can be derived dynamically. This was one of the main reasons for creatingSymbolicDistributions in the first place.Closes #5533
Checklist
Major / Breaking Changes
Bugfixes / New features
Docs / Maintenance
SymbolicRVs to encapsulate graphs from distribution factoriesSymbolicDistributionclass in favor ofDistributionchange_rv_sizewas removed in favor of the more generalchange_dist_size_LKJCholeskyCovandGaussianRandomWalkare nowSymbolicRVs