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 specialization for subtraction and addition with negative terms #549

Merged
merged 3 commits into from
Oct 18, 2022

Conversation

ricardoV94
Copy link
Contributor

@ricardoV94 ricardoV94 commented Aug 6, 2021

x - (-y) -> x + y
x + (-y) -> x - y 
-x + y -> y - x

Adding these during canonicalization conflicted with a couple of other rewrites.

  1. local_neg_to_mul tries to convert -x -> -1. * x, so that the local_mul_canonizer can do its thing.
  2. When it comes to constants, the local_add_canonizer will try to place them on the left of the graph for "constant folding" and leave them as an add, even if they are negative. For example x - 5 -> -5 + x.

The new rewrites were registered or designed not to conflict with these preexisting rewrites

Closes #546

@ricardoV94 ricardoV94 force-pushed the sub_neg_rewrite branch 2 times, most recently from 6e97aa6 to 391e2cf Compare August 6, 2021 17:47
aesara/tensor/math_opt.py Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Aug 6, 2021

Codecov Report

Merging #549 (1846741) into main (77df667) will increase coverage by 0.00%.
The diff coverage is 96.00%.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #549   +/-   ##
=======================================
  Coverage   74.09%   74.10%           
=======================================
  Files         174      174           
  Lines       48605    48622   +17     
  Branches    10340    10348    +8     
=======================================
+ Hits        36014    36030   +16     
  Misses      10305    10305           
- Partials     2286     2287    +1     
Impacted Files Coverage Δ
aesara/tensor/rewriting/math.py 86.34% <96.00%> (+0.08%) ⬆️

Copy link
Member

@brandonwillard brandonwillard left a comment

Choose a reason for hiding this comment

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

This looks really good! I have some comments/questions about the special DimShuffle logic, but that's it.

aesara/tensor/math_opt.py Outdated Show resolved Hide resolved
aesara/tensor/math_opt.py Outdated Show resolved Hide resolved
aesara/tensor/math_opt.py Outdated Show resolved Hide resolved
aesara/tensor/math_opt.py Outdated Show resolved Hide resolved
@ricardoV94
Copy link
Contributor Author

ricardoV94 commented Aug 7, 2021

I should take a look if somethings can be removed from here after this PR:

local_one_plus_erf = PatternSub(

There seems to be a lot of redundancy going on

@ricardoV94 ricardoV94 force-pushed the sub_neg_rewrite branch 3 times, most recently from 9e6b7e6 to d0b208a Compare August 9, 2021 07:40
@ricardoV94 ricardoV94 marked this pull request as draft August 9, 2021 10:11
@ricardoV94 ricardoV94 force-pushed the sub_neg_rewrite branch 2 times, most recently from 6142a8e to 164578b Compare August 9, 2021 19:52
@ricardoV94 ricardoV94 marked this pull request as ready for review August 9, 2021 19:52
@ricardoV94 ricardoV94 changed the title Add canonicalizations for subtraction and addition with negative terms Add specialization for subtraction and addition with negative terms Aug 9, 2021
@ricardoV94 ricardoV94 marked this pull request as draft August 10, 2021 08:07
@ricardoV94 ricardoV94 marked this pull request as ready for review January 19, 2022 08:48
@ricardoV94 ricardoV94 marked this pull request as draft February 20, 2022 17:40
@ricardoV94 ricardoV94 marked this pull request as ready for review February 22, 2022 11:04
@ricardoV94 ricardoV94 force-pushed the sub_neg_rewrite branch 2 times, most recently from aeaf64d to be0c437 Compare March 16, 2022 17:29
Copy link
Member

@brandonwillard brandonwillard left a comment

Choose a reason for hiding this comment

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

Looks like CI was stuck, so I rebased it.

@rlouf
Copy link
Member

rlouf commented Oct 17, 2022

Rebased the branch on main and resolved the merge conflicts as this looks still relevant.

@rlouf rlouf merged commit 2cf617d into aesara-devs:main Oct 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Create canonicalization for sub(x, neg(y))
3 participants