Skip to content
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 support for power transforms #210

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

brandonwillard
Copy link
Member

@brandonwillard brandonwillard commented Dec 5, 2022

This PR adds support for general power transforms and replaces #184. It uses a positive support in cases of ambiguity.

In its current form, it appears to work as a replacement for at.reciprocal, but the $Z = X^2$, where $X \sim \operatorname{N}\left(0, 1\right)$, test example fails.

  • Fix and add more tests

@brandonwillard brandonwillard marked this pull request as draft December 5, 2022 05:04
@brandonwillard brandonwillard added enhancement New feature or request graph rewriting Involves the implementation of rewrites to Aesara graphs rv-transforms Involves transforms applied to random variables op-probability Involves the implementation of log-probabilities for Aesara `Op`s labels Dec 5, 2022
@rlouf
Copy link
Member

rlouf commented Dec 5, 2022

Just a note that #184 originally introduced a rewrite for at.square as well, but I think it would be better to only register at.pow here and make sure they're equivalent Aesara-side: aesara-devs/aesara#1213

It uses a positive support in cases of ambiguity.

Another arguments in favor of propagating domain information; we shouldn't have to make assumptions here.

return -2 * at.log(value)
from aeppl.logprob import xlogy0

power = self.transform_args_fn(*inputs)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't you need to use inv_power = at.reciprocal(power) instead of power in the expression of the jacobian?

@@ -422,8 +422,20 @@ def transform(measurable_input, *other_inputs):
def measurable_reciprocal(fgraph, node):
"""Rewrite a `reciprocal` node to a `MeasurableVariable`."""

def transform(measurable_input, *other_inputs):
return ReciprocalTransform(), (measurable_input,)
new_node = at.power(node.inputs[0], at.as_tensor(-1)).owner
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the advantage of using pow to represent reciprocal here (besides a few less lines of code)? I feel such equivalences should be handled at the Aesara-level, either by aliasing or a canonicalization pass.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request graph rewriting Involves the implementation of rewrites to Aesara graphs op-probability Involves the implementation of log-probabilities for Aesara `Op`s rv-transforms Involves transforms applied to random variables
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants