pymc icon indicating copy to clipboard operation
pymc copied to clipboard

Default rewrites in inner graph

Open aerubanov opened this issue 2 years ago • 10 comments

This PR will add implementation of default rewrites for nodes with inner fgraph and related with issue #6697. For now I just added new test cases


:books: Documentation preview :books:: https://pymc--6996.org.readthedocs.build/en/6996/

aerubanov avatar Nov 07 '23 12:11 aerubanov

Codecov Report

Merging #6996 (b8fde47) into main (547bcb4) will decrease coverage by 4.25%. Report is 11 commits behind head on main. The diff coverage is 94.11%.

:exclamation: Current head b8fde47 differs from pull request most recent head 0c80a68. Consider uploading reports for the commit 0c80a68 to get more accurate results

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #6996      +/-   ##
==========================================
- Coverage   92.17%   87.92%   -4.25%     
==========================================
  Files         101      101              
  Lines       16849    16891      +42     
==========================================
- Hits        15530    14851     -679     
- Misses       1319     2040     +721     
Files Coverage Δ
pymc/logprob/utils.py 96.19% <94.11%> (-1.31%) :arrow_down:

... and 8 files with indirect coverage changes

codecov[bot] avatar Nov 07 '23 13:11 codecov[bot]

The tests are great!

ricardoV94 avatar Nov 09 '23 10:11 ricardoV94

Logic for removing CheckParameterValues from Scan is very similar to one we recently added in #6873 for moment calculation in Scan, so may be I will move it to some helper function later

aerubanov avatar Nov 14 '23 10:11 aerubanov

There is some refactoring going on in #6976 that will impact the location of these changes.

One thing we need to think about is nested Scans/OpFromGraph, I think this approach will only work for the first level.

PS: Agree with refactoring the functionality if it's so similar

ricardoV94 avatar Nov 14 '23 10:11 ricardoV94

I added recursive check for nested Scans. For now it is only remove CheckParameterValues nodes from fgraph, but I will add replacement by ninf next step

aerubanov avatar Nov 15 '23 13:11 aerubanov

@ricardoV94 I added logic for replacement CheckParameterValues by ninf in Scan and rebased my branch. Now I am going to add support for OpFromGraph and want to extract common logic for rewrites in inner fgraphs - probably some helper function. I think we can add some general solution for inner fgraphs rewriting.

aerubanov avatar Nov 18 '23 15:11 aerubanov

I added support for replacements in OpFromGraph

aerubanov avatar Nov 21 '23 12:11 aerubanov

I move some common logic into InnerGraphRewrite class

aerubanov avatar Nov 30 '23 16:11 aerubanov

@ricardoV94 Could you please take a look when you will have some time?

aerubanov avatar Nov 30 '23 16:11 aerubanov

I will re-implement this with WalkingNestedGraphRewriter after merging https://github.com/pymc-devs/pytensor/pull/556

aerubanov avatar Dec 14 '23 17:12 aerubanov