Skip to content

Allow fill_sink rewrite to accomodate changes in broadcastability #785

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 1 commit into from
May 28, 2024

Conversation

ricardoV94
Copy link
Member

@ricardoV94 ricardoV94 commented May 27, 2024

Description

The strict assumption that one of the existing variables could make the rewritten output broadcastable pattern match the original one, led to a failure in pymc-devs/pymc#7327 (https://github.com/pymc-devs/pymc/actions/runs/9253581078/job/25453566092?pr=7327#step:7:265)

This is related to #408, the sooner we fix that the better, as this kind of problems keep popping all the time, and makes graph subject to changes in behavior depending on whether rewrites are performed or not

Related Issue

  • Closes #
  • Related to #

Checklist

Type of change

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

@ricardoV94 ricardoV94 added bug Something isn't working graph rewriting labels May 27, 2024
@ricardoV94 ricardoV94 requested review from aseyboldt and ferrine May 27, 2024 16:11
@ricardoV94
Copy link
Member Author

ricardoV94 commented May 27, 2024

Should graph_replace (which is the way this "mis-behaved" graph was created in the PyMC test) rebuild the downstream graph when strict=False and the new inputs have different static types (like clone_replace does?). IIRC we changed from using graph_replace to clone_replace in nutpie because graph_replace was not doing this?

Had it rebuild the downstream nodes, the problem wouldn't have shown up (I still like the new rewrite code better).

Copy link

codecov bot commented May 27, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 80.85%. Comparing base (5f374db) to head (2615da0).
Report is 210 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #785   +/-   ##
=======================================
  Coverage   80.85%   80.85%           
=======================================
  Files         162      162           
  Lines       47045    47043    -2     
  Branches    11514    11514           
=======================================
- Hits        38039    38038    -1     
  Misses       6750     6750           
+ Partials     2256     2255    -1     
Files Coverage Δ
pytensor/tensor/rewriting/basic.py 93.89% <100.00%> (+0.14%) ⬆️

... and 1 file with indirect coverage changes

Copy link
Member

@aseyboldt aseyboldt left a comment

Choose a reason for hiding this comment

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

Sounds good!

@ricardoV94 ricardoV94 merged commit fc21336 into pymc-devs:main May 28, 2024
56 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working graph rewriting
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants