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 rewrite to merge consecutive Joined Subtensors #760

Merged
merged 1 commit into from
Apr 29, 2022

Conversation

ricardoV94
Copy link
Contributor

I was playing a bit with this kind of rewrites recently, which appeared for me when undoing at.diff operations which reduce the first axis index.

concatenate(([x[:1], x[1:])) -> x

I am not sure this is a useful rewrite to have in the codebase, let me know what you think.
If this is useful, it will probably still need a bit of cleanup, as I've never worked with SubTensors before.

@ricardoV94 ricardoV94 force-pushed the joined_subtensors branch 2 times, most recently from 912d867 to 00da564 Compare January 17, 2022 17:36
@codecov
Copy link

codecov bot commented Jan 17, 2022

Codecov Report

Merging #760 (ccd8360) into main (c19ac79) will increase coverage by 0.00%.
The diff coverage is 90.24%.

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #760   +/-   ##
=======================================
  Coverage   78.91%   78.92%           
=======================================
  Files         152      152           
  Lines       47657    47698   +41     
  Branches    10852    10861    +9     
=======================================
+ Hits        37609    37646   +37     
- Misses       7548     7550    +2     
- Partials     2500     2502    +2     
Impacted Files Coverage Δ
aesara/tensor/subtensor_opt.py 86.06% <90.24%> (+0.23%) ⬆️

aesara/tensor/subtensor_opt.py Outdated Show resolved Hide resolved
@ricardoV94 ricardoV94 force-pushed the joined_subtensors branch 2 times, most recently from f5441ae to cd6fd7f Compare January 18, 2022 09:16
@brandonwillard brandonwillard changed the title Add rewrite to merge consecutive joined subtensors Add rewrite to merge consecutive Joined Subtensors Jan 19, 2022
aesara/tensor/subtensor_opt.py Outdated Show resolved Hide resolved
aesara/tensor/subtensor_opt.py Outdated Show resolved Hide resolved
aesara/tensor/subtensor_opt.py Outdated Show resolved Hide resolved
@ricardoV94 ricardoV94 force-pushed the joined_subtensors branch 4 times, most recently from 168a2ce to f52bce5 Compare January 19, 2022 17:17
@ricardoV94 ricardoV94 force-pushed the joined_subtensors branch 2 times, most recently from d455cef to 675ac90 Compare February 20, 2022 17:38
aesara/tensor/subtensor_opt.py Outdated Show resolved Hide resolved
aesara/tensor/subtensor_opt.py Outdated Show resolved Hide resolved
brandonwillard
brandonwillard previously approved these changes Apr 28, 2022
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.

A conflict needs to be resolved; otherwise, this looks good to merge.

@ricardoV94
Copy link
Contributor Author

ricardoV94 commented Apr 28, 2022

Fixed a wrong assumption that we could perform this rewrite as long as the steps were equal.

Also got rid of is comparisons and simplified the tests.

@brandonwillard brandonwillard merged commit ea5401c into aesara-devs:main Apr 29, 2022
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants