-
-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Refactor Mixture distribution for V4 #5438
Conversation
Codecov Report
@@ Coverage Diff @@
## main #5438 +/- ##
==========================================
+ Coverage 87.29% 88.15% +0.86%
==========================================
Files 81 81
Lines 14247 14238 -9
==========================================
+ Hits 12437 12552 +115
+ Misses 1810 1686 -124
|
ce37fd4
to
a349d35
Compare
@Sayam753 could you give some context on what was the idea with Now that we have meta information about the components (ndim_supp, ndims_params), it is perhaps no longer needed? |
The legacy
@lucianopaz is the magician behind this distribution. |
If I understand correctly, I think we can infer the correct mixture axis from the meta information we have now. That's my TODO point above. So far, I have just been hacking it by trial and error. Do you think there is inherent ambiguity still? Even considering ndim_supp, ndims_params, shape_from_params... and whatever else methods we have to reason about shapes of RandomVariables? |
Figured out the weights shape padding/broadcasting thanks tho @lucianopaz! |
fac451f
to
3a39b82
Compare
pymc/tests/test_mixture.py
Outdated
# Expected to fail if comp_shape is not provided, | ||
# nd is multidim and it does not broadcast with ncomp. If by chance | ||
# it does broadcast, an error is raised if the mixture is given | ||
# observed data. | ||
# Furthermore, the Mixture will also raise errors when the observed | ||
# data is multidimensional but it does not broadcast well with | ||
# comp_dists. |
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.
This no longer seems to be a problem. @lucianopaz can you confirm that if the current tests pass, this is indeed fine?
b2d19a9
to
8d665fa
Compare
@OriolAbril Any idea why the |
There should be spaces both before and after the colon that separates parameter name and type |
7e77630
to
dfdce06
Compare
Moments are also ready thanks to @larryshamalama. This is ready for review and merge 🚀 |
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.
Looks great 🙂 Thanks for leading the effort @ricardoV94
Mixtures now use an `OpFromGraph` that encapsulates the Aesara random method. This is used so that logp can be easily dispatched to the distribution without requiring involved pattern matching. The Mixture random and logp methods now fully respect the support dimensionality of its components, whereas previously only the logp method did, leading to inconsistencies between the two methods. In the case where the weights (or size) indicate the need for more draws than what is given by the component distributions, the latter are resized to ensure there are no repeated draws. This refactoring forces Mixture components to be basic RandomVariables, meaning that nested Mixtures or Mixtures of Symbolic distributions (like Censored) are not currently possible. Co-authored-by: Larry Dong <[email protected]>
* Emphasize equivalency between iterable of components and single batched component * Add example with mixture of two distinct distributions * Add example with multivariate components
The two tests relied on implicit behavior of V3, where the dimensionality of the weights implied the support dimension of mixture distribution. This, however, led to inconsistent behavior between the random method and the logp, as the latter did not enforce this assumption, and did not distinguish if values were mixed across the implied support dimension. In this refactoring, the support dimensionality of the component variables determines the dimensionality of the mixture distribution, regardless of the weights. This leads to consistent behavior between the random and logp methods as asserted by the new checks. Future work will explore allowing the user to specify an artificial support dimensionality that is higher than the one implied by the component distributions, but this is for now not possible.
Behavior is now implemented in Mixture
This PR is an attempt to refactor (Marginalized) Mixture distributions for V4
Changes
There are two big changes in how Mixture works compared to V3:
I am exploring adding a keyword argument to override the support dimensionality in a follow-up PR. I managed to do this for the random method, but haven't had time yet to figure out what needs to be changed in the logp method.
Closes #4781