Skip to content

Merge consecutive reduces #888

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 3 commits into from
Oct 8, 2024

Conversation

ricardoV94
Copy link
Member

@ricardoV94 ricardoV94 commented Jul 5, 2024

Description

This PR does some cleanup and extends reduction rewrites. Check each commit for the individual changes. Summary:

  1. Remove list of ALL_REDUCE. AFAICT the tracks machinery already handles subclasses automatically. It did not always do it, which is probably why the list existed
  2. Stop defining Min as negative of Max, to later undo it and actually use Min. I think this is just some historical accident? Reverted, too many things depend on this for me to tackle now: Reconsider min implementation as negative of max #904
  3. Generalize a chain of reductions of the same type to any CAReduce. It was previously limited only to Sum and Prod, but I can't see a good reason for it. This showed up in vectorized logps where All reductions (related to the parameter checks) were not being combined.
  4. Generalize local_reduce_join to any axis and regardless of whether the inputs along that axis are broadcastable because of expand_dims or any other reason. It shouldn't matter, as long as we can squeeze it out.

Checklist

Type of change

  • New feature / enhancement
  • Bug fix
  • Documentation
  • Maintenance
  • Other (please specify):

@ricardoV94 ricardoV94 requested a review from jessegrabowski July 5, 2024 19:05
@ricardoV94 ricardoV94 force-pushed the merge_consecutive_reduces branch from 973ac9f to 7174d7d Compare July 5, 2024 19:07
@ricardoV94 ricardoV94 removed the request for review from jessegrabowski July 5, 2024 19:32
@ricardoV94 ricardoV94 marked this pull request as draft July 5, 2024 19:32
@ricardoV94 ricardoV94 mentioned this pull request Jul 8, 2024
11 tasks
@ricardoV94 ricardoV94 force-pushed the merge_consecutive_reduces branch from 7174d7d to 7f8a913 Compare July 8, 2024 11:21
@ricardoV94 ricardoV94 force-pushed the merge_consecutive_reduces branch from 7f8a913 to 2047fee Compare July 8, 2024 14:48
@ricardoV94 ricardoV94 marked this pull request as ready for review July 8, 2024 14:48
@ricardoV94 ricardoV94 requested a review from jessegrabowski July 8, 2024 14:49
@ricardoV94 ricardoV94 force-pushed the merge_consecutive_reduces branch from 2047fee to e013cd4 Compare July 8, 2024 15:00
@ricardoV94 ricardoV94 force-pushed the merge_consecutive_reduces branch from e013cd4 to 58b8c93 Compare July 8, 2024 15:33
Copy link

codecov bot commented Jul 8, 2024

Codecov Report

Attention: Patch coverage is 88.52459% with 7 lines in your changes missing coverage. Please review.

Project coverage is 81.25%. Comparing base (8e0958a) to head (58b8c93).
Report is 218 commits behind head on main.

Files with missing lines Patch % Lines
pytensor/tensor/rewriting/math.py 88.52% 3 Missing and 4 partials ⚠️
Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #888   +/-   ##
=======================================
  Coverage   81.24%   81.25%           
=======================================
  Files         170      170           
  Lines       46920    46922    +2     
  Branches    11482    11480    -2     
=======================================
+ Hits        38120    38125    +5     
+ Misses       6600     6599    -1     
+ Partials     2200     2198    -2     
Files with missing lines Coverage Δ
pytensor/tensor/rewriting/math.py 89.61% <88.52%> (+0.20%) ⬆️

... and 2 files with indirect coverage changes

@ricardoV94 ricardoV94 merged commit 2086aeb into pymc-devs:main Oct 8, 2024
58 of 59 checks passed
@ricardoV94 ricardoV94 added enhancement New feature or request and removed maintenance labels Oct 8, 2024
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