-
-
Notifications
You must be signed in to change notification settings - Fork 11
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
Add relation for recentering and rescaling #31
Conversation
0bd0d22
to
9d24e90
Compare
Codecov Report
@@ Coverage Diff @@
## main #31 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 4 5 +1
Lines 241 313 +72
Branches 19 20 +1
=========================================
+ Hits 241 313 +72
Continue to review full report at Codecov.
|
d64bf4b
to
7fddeca
Compare
I tried to use
The current value of the node is node
# normal_rv{0, (0, 0), floatX, False}(RandomGeneratorSharedVariable(<Generator(PCG64) at 0x7F6D2BB1B140>), TensorConstant{[]}, TensorConstant{11}, normal_rv{0, (0, 0), floatX, False}.out, halfcauchy_rv{0, (0, 0), floatX, False}.out) kanren seems to have successully matched the input expression with its non-centered equivalent: replacements
# [Elemwise{add,no_inplace}.0]
replacements[0].owner
# Elemwise{add,no_inplace}(normal_rv{0, (0, 0), floatX, False}.out, Elemwise{mul,no_inplace}.0)
replacements[0].owner.inputs
# [normal_rv{0, (0, 0), floatX, False}.out, Elemwise{mul,no_inplace}.0]
replacements[0].owner.inputs[1].owner
# Elemwise{mul,no_inplace}(halfcauchy_rv{0, (0, 0), floatX, False}.out, normal_rv{0, (0, 0), floatX, False}.out) So the issue seems to come from the fact that It looks like this is something that should be fixed in |
fact(location_scale_family, at.random.laplace) | ||
fact(location_scale_family, at.random.logistic) | ||
fact(location_scale_family, at.random.normal) | ||
|
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.
Is this list meant to be exhaustive? If so then I'd add the uniform and t distributions as well.
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 just added the ones on the top of my head when I wrote the code, but good point
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.
Don't forget, a lot of these distributions are given by more general distribution(s) (e.g. generalized hyperbolic distributions), so we could simplify the recentering/rescaling rewrite implementations via the use of intermediate rewrites that convert them to their generalized distribution(s).
7fddeca
to
ff705b8
Compare
I fixed the problem related to the fact that It does not work yet in the other direction, I added a test that currently fails. This may be another occurence of the issue we observed in #29, in which case I will mark this test as |
ef61402
to
a9379aa
Compare
The problem indeed comes from what we observed in #29. I'm opening an issue on |
a9379aa
to
4fcf34d
Compare
6aad4d7
to
0da0387
Compare
@brandonwillard this is ready to merge. I will open a PR to make sure that the backward transformation works here and for the beta-binomial transformation once aesara-devs/aesara#1002 has been addressed. |
We can remove the |
In this PR we add a relation that represents loc-scale transformations (the relation between the "centered" and "non-centered" parametrization). The performance of some sampling algorithms (eg HMC) can be affected by the parametrization of the model provided by the user (see this example).
We would like
aemcmc
to be aware of the different parametrizations so users don't have to worry of these implementation details.I see that the Symbolic-PyMC implementation excludes the cases where
loc=0
andsd=1
but I am not sure that it is necessary since the relation still holds in this case (even though pointless but algebraic simplification after transformation should take care of this).Note : We should implement the location and scale transformations separately: distributions like the exponential distributions are part of the scale but not location-scale family.
Closes #5