-
Notifications
You must be signed in to change notification settings - Fork 131
Rewrite products of exponents as exponent of sum #186
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
Merged
Merged
Changes from all commits
Commits
Show all changes
8 commits
Select commit
Hold shift + click to select a range
a52f8bc
pytensor-54: Rewrite products of exponents as exponent of sum. Rewrit…
tamastokes 28fdc86
pytensor-54: Handle properly the scenarios where a Mul node has more …
tamastokes 8466acd
pytensor-54: Rewrite a^x * a^y to a^(x+y)
tamastokes 799722b
pytensor-54: Rename functions according to naming conventions. Remove…
tamastokes 426f0c0
pytensor-54: Removed yet another redundant check
tamastokes 5ac8d92
pytensor-54: Exempt fusion rewrites in the test cases of local_mul_ex…
tamastokes db41813
pytensor-54: Attempt to fix test failing in float32 mode due to impli…
tamastokes 28d1ca6
pytensor-54: Explicitly include the tested rewrites in the tests, so …
tamastokes File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
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.
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.
Rethinking this, it may make sense to register these new rewrites in canonicalize 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.
Hi, after looking around how it's resolved elsewhere and some experimentation with different options, I've ended up adding the following line at the beginning of the test function, which seems to have resolved the issue on my local runs in both with and without the FAST_COMPILE flag.
mode = get_mode("FAST_RUN") if config.mode == "FAST_COMPILE" else get_default_mode()
(Obviously, with passing the mode parameter to all function() calls)
If you're ok with this, on which branch do you want me to commit this change? The original one or the the one you had rebased that branch earlier?
Also, do you want me to add back the @register_canonicalize decorators?
Thanks!
Uh oh!
There was an error while loading. Please reload this page.
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 think a more clean solution is to use
mode=get_default_mode().excluding("fusion")
Then the tests will also be more straightforward because you don't need to check the graph inside the Composites
And yes, let's try to add the
register_canonicalize
to these rewrites and see if it doesn't break anything.Uh oh!
There was an error while loading. Please reload this page.
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.
An interesting side effect of making the rewrites canonical is that one of the test cases in test_math.py::TestSigmoidRewrites::test_local_sigm_times_exp breaks, as now my rewrites takes precedence over the sigma_times_exp rewrites. More specifically,
sigma(x) * sigma(-y) * (-exp(-x)) * exp(xy) * exp(y)
is supposed to become
(-1) * sigma(-x) * sigma(y) * exp(xy)
but my rewrites contracts the exp-s before the sigma*exp rewrite could take action, and the result becomes
(-1) * exp(xy+y-x) * sigma(x) * sigma(-y)
So, I guess, this is a trade-off, how should we resolve this? (Having run all tests, this is the only one the canonicalisation broke.)
Uh oh!
There was an error while loading. Please reload this page.
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.
We could maybe extend the
sigma_times_exp
rewrite to check if the redundant term is somewhere inside the exponentiation (instead of being all there is)?exp(y - x) * sigmoid(x) -> exp(y) * sigmoid(x)
Does that make sense? If the canonicalize doesn't do it we might need to represent the contents of the exponent as a flat a series of additions (with negated terms instead of substitution) so that we can match them more easily.
Might have other ramifications I am not seeing.
Uh oh!
There was an error while loading. Please reload this page.
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.
Looking at the rewrite, such change does not seem trivial, so let's leave as is: don't canonicalize.
We can open a new issue to investigate if it's worth it.
Let's just change the mode thing in the tests.
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'm wondering how frequent / marginal such use cases are. Are there any kind of statistics available on how often real world use cases trigger which rewrites?
Meanwhile, I've just committed the changes for the tests, hope they'll be fine this time.
Thanks!
Uh oh!
There was an error while loading. Please reload this page.
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.
Unfortunately, we have no telemetrics to study the types of graphs users create :D