Graph replace #66

ferrine · 2022-11-30T13:03:42Z

Implementing graph replace that preserves references from the inherited graph (#19)

Here are a few important guidelines and requirements to check before your PR can be merged:

There is an informative high-level description of the changes.
The description and/or commit message(s) references the relevant GitHub issue(s).
pre-commit is installed and set up.
The commit messages follow these guidelines.
The commits correspond to relevant logical changes, and there are no commits that fix changes introduced by other commits in the same branch/BR.
There are tests covering the changes introduced in the PR.

Don't worry, your PR doesn't need to be in perfect order to submit it. As development progresses and/or reviewers request changes, you can always rewrite the history of your feature/PR branches.

If your PR is an ongoing effort and you would like to involve us in the process, simply make it a draft PR.

aseyboldt

I like this a lot, but maybe we can improve the condation_subset function a bit, or at least make it easier to understand what it actually does?

pytensor/graph/basic.py

ferrine · 2022-12-01T06:55:22Z

I would also slightly refactor is_in_ancestors though, my implementation for is_dependent_on does exactly the same thing but for nodes. Adding singledispatch would make API more consistent, But it can go separately

tests/graph/test_utils.py

tests/graph/test_basic.py

ferrine · 2022-12-01T12:16:37Z

I've found that there are realistic cases where one replacement depends on another one. So far fg does these replacements inplace #74 and it is unsafe to apply subsequent replaces. This is the reason I have several passes in the graph_replace .

The question what should user expect if some of the replacements were not applied?

ferrine · 2022-12-01T13:35:10Z

Ready for review

pytensor/graph/utils.py

codecov-commenter · 2022-12-01T14:44:42Z

Codecov Report

Merging #66 (aa51011) into main (491f93e) will increase coverage by 0.05%.
The diff coverage is 87.15%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main      #66      +/-   ##
==========================================
+ Coverage   74.22%   74.28%   +0.05%     
==========================================
  Files         174      176       +2     
  Lines       48734    48991     +257     
  Branches    10367    10414      +47     
==========================================
+ Hits        36175    36392     +217     
- Misses      10272    10293      +21     
- Partials     2287     2306      +19

Impacted Files	Coverage Δ
pytensor/misc/ordered_set.py	`79.80% <ø> (ø)`
pytensor/tensor/nnet/corr.py	`16.81% <0.00%> (ø)`
pytensor/ifelse.py	`51.42% <66.66%> (+0.13%)`	⬆️
pytensor/link/numba/dispatch/extra_ops.py	`92.24% <70.21%> (-5.77%)`	⬇️
pytensor/link/numba/dispatch/scalar.py	`94.44% <75.00%> (+7.02%)`	⬆️
pytensor/graph/replace.py	`84.61% <84.61%> (ø)`
pytensor/link/numba/dispatch/cython_support.py	`86.95% <86.95%> (ø)`
pytensor/link/numba/dispatch/basic.py	`90.06% <95.83%> (-2.62%)`	⬇️
pytensor/graph/basic.py	`88.10% <97.05%> (+0.43%)`	⬆️
pytensor/compile/builders.py	`77.11% <100.00%> (+0.05%)`	⬆️
... and 14 more

ferrine · 2022-12-05T10:19:09Z

@ricardoV94 @aseyboldt anything else left on the PR?

pytensor/graph/basic.py

aseyboldt · 2022-12-05T23:27:04Z

@ricardoV94 @aseyboldt anything else left on the PR?

I'm still not 100% sure about the helper function. Can we at least make it private, in case we want to further specify/change its exact behavior?

ricardoV94

I really like out this is shaping. There are a bunch of places in pymc and pytensor I can see the use for this already.

Left some small suggestions and questions.

pytensor/graph/utils.py

pytensor/graph/basic.py

ricardoV94 · 2022-12-06T08:05:26Z

@ricardoV94 @aseyboldt anything else left on the PR?

I'm still not 100% sure about the helper function. Can we at least make it private, in case we want to further specify/change its exact behavior?

I don't see the reason for that. It's an alternative to clone_replace without the clone, I think it's a safe starting point to share the same external API. The internal helper functions used by it can be made private.

pytensor/graph/utils.py

ricardoV94 · 2022-12-06T08:45:02Z

pytensor/graph/basic.py

+def variable_is_in_ancestors(
+    node: Variable, check: Union[Variable, Collection[Variable]]
+) -> bool:
+    """Determine if `node` is in the graph given by any in check.
+    Parameters
+    ----------
+    node: Variable
+        Node to check
+    check: Collection[Variable]
+        Nodes to check dependency on
+    Returns
+    -------
+    bool
+    """
+    if not isinstance(check, Collection):
+        check = {check}
+    else:
+        check = set(check)
+    return any(interim in check for interim in ancestors([node], blockers=check))


I would perhaps inline this function, I think it's simple enough for users to figure out. We can add it later if there's a demand for it.

I am not totally set, so feel free to insist in keeping it

I would like to merge this with is_in_ancestors so it is more polymorphic

NodeT can be either Variable or Apply is_in_ancestors(node[NodeT], search: Union[List[NodeT], NodeT]) -> bool

What I was saying is I don't think we need this helper function at all. Ancestors gives you an iterable and you check if it is in it. I don't see why we need a user-facing function for the one-liner

Okay, we already had one for nodes, so that opens a valid precedent for variables. I think we should keep separate. There's a subtle difference between checking for nodes and variables.

I renamed the is_in_ancestors to apply_in_ancestors and unified the api with variable_in_ancestors so they are semantically the same. Should we bother with deprecation warnings and add a fallback?

Yes I think we should bother with deprecation warning for this case. It's not too much trouble either.

pytensor/graph/utils.py

pytensor/graph/basic.py

pytensor/graph/utils.py

aseyboldt · 2022-12-06T16:24:55Z

I don't see the reason for that. It's an alternative to clone_replace without the clone, I think it's a safe starting point to share the same external API. The internal helper functions used by it can be made private.

I only meant the truncated_graph_inputs function, not graph_replace.

ferrine · 2022-12-06T18:47:12Z

I don't see the reason for that. It's an alternative to clone_replace without the clone, I think it's a safe starting point to share the same external API. The internal helper functions used by it can be made private.

I only meant the truncated_graph_inputs function, not graph_replace.

Don't see any reason to make the function private, there are many ways to use it outside pytensor

Just a quick example I could imagine

candidates = set(model.basic_RVs)
conditioned_on = {
    rv: candidates & truncated_graph_inputs([rv], candidates - {rv}) 
    for rv in cmodel.basic_RVs
}

Another thing that there is no private function in the basic.py, so any function is assumed to be freely accessible

pytensor/graph/basic.py

pytensor/graph/utils.py

tests/graph/test_utils.py

pytensor/graph/basic.py

ricardoV94

@ferrine I am happy with the code! Just some corrections/nitpicks below. Also, can you clean the git history + add deprecation for is_in_ancestors (if you didn't already)?

pytensor/graph/basic.py

ricardoV94 · 2022-12-07T12:56:33Z

tests/graph/test_replace.py

+        )
+
+
+def test_graph_replace():


Can you group these in a class like clone_replace

ricardoV94

Looks great @ferrine. Only one nitpick: you have a typo in the first commit move cclone_replace to a separate file (double c in cclone_...) 9822a56

ferrine · 2022-12-09T20:43:16Z

Who will press the green button?

ferrine · 2022-12-09T20:46:56Z

Thanks @ricardoV94 for your tremendous help and support with this pr. Also thanks @aseyboldt for pointing out algorithmic issues when it started out

aseyboldt · 2022-12-09T20:52:04Z

Looking forward to using this!
Good work, I guess the honor is yours :-)

ricardoV94 · 2022-12-09T21:28:56Z

pytensor/graph/basic.py

+            dependent = variable_depends_on(node, blockers - {node})
+            # ancestors to include that are present in the graph (not disconnected)
+            # should be added to truncated_inputs
+            truncated_inputs.append(node)


@ferrine I think we can end up with duplicated inputs here. If another node has duplicated truncated inputs?

ricardoV94 · 2022-12-09T21:33:20Z

Good work, I guess the honor is yours :-)

Oops, I didn't see these messages!

ferrine force-pushed the graph-replace branch from 94ae188 to c4b00d6 Compare November 30, 2022 14:57

ferrine requested a review from ricardoV94 November 30, 2022 16:18

aseyboldt reviewed Nov 30, 2022

View reviewed changes

pytensor/graph/basic.py Outdated Show resolved Hide resolved

ferrine force-pushed the graph-replace branch from 97fc000 to a54c53d Compare December 1, 2022 08:17

ricardoV94 added enhancement New feature or request graph objects and removed graph objects labels Dec 1, 2022

ricardoV94 reviewed Dec 1, 2022

View reviewed changes

tests/graph/test_utils.py Outdated Show resolved Hide resolved

ricardoV94 reviewed Dec 1, 2022

View reviewed changes

tests/graph/test_basic.py Outdated Show resolved Hide resolved

ferrine force-pushed the graph-replace branch 2 times, most recently from 6de138d to f009a76 Compare December 1, 2022 12:06

ricardoV94 reviewed Dec 1, 2022

View reviewed changes

pytensor/graph/utils.py Outdated Show resolved Hide resolved

pytensor/graph/utils.py Outdated Show resolved Hide resolved

ricardoV94 reviewed Dec 5, 2022

View reviewed changes

pytensor/graph/basic.py Outdated Show resolved Hide resolved

ricardoV94 requested changes Dec 6, 2022

View reviewed changes

pytensor/graph/utils.py Outdated Show resolved Hide resolved

pytensor/graph/utils.py Outdated Show resolved Hide resolved

pytensor/graph/utils.py Outdated Show resolved Hide resolved

pytensor/graph/basic.py Outdated Show resolved Hide resolved

pytensor/graph/basic.py Show resolved Hide resolved

ricardoV94 requested changes Dec 6, 2022

View reviewed changes

pytensor/graph/basic.py Outdated Show resolved Hide resolved

pytensor/graph/basic.py Show resolved Hide resolved

ricardoV94 reviewed Dec 6, 2022

View reviewed changes

pytensor/graph/utils.py Outdated Show resolved Hide resolved

ricardoV94 reviewed Dec 6, 2022

View reviewed changes

pytensor/graph/utils.py Outdated Show resolved Hide resolved

ricardoV94 reviewed Dec 6, 2022

View reviewed changes

pytensor/graph/basic.py Outdated Show resolved Hide resolved

pytensor/graph/basic.py Show resolved Hide resolved

pytensor/graph/basic.py Outdated Show resolved Hide resolved

pytensor/graph/basic.py Outdated Show resolved Hide resolved

pytensor/graph/utils.py Outdated Show resolved Hide resolved

ricardoV94 reviewed Dec 7, 2022

View reviewed changes

pytensor/graph/basic.py Outdated Show resolved Hide resolved

ricardoV94 reviewed Dec 7, 2022

View reviewed changes

pytensor/graph/basic.py Outdated Show resolved Hide resolved

ricardoV94 reviewed Dec 7, 2022

View reviewed changes

pytensor/graph/basic.py Outdated Show resolved Hide resolved

ricardoV94 requested changes Dec 7, 2022

View reviewed changes

pytensor/graph/basic.py Outdated Show resolved Hide resolved

ferrine force-pushed the graph-replace branch 5 times, most recently from 5cd9cd3 to c9fd99b Compare December 7, 2022 12:11

ricardoV94 requested changes Dec 7, 2022

View reviewed changes

ferrine force-pushed the graph-replace branch 2 times, most recently from 73007a6 to 17e189f Compare December 9, 2022 14:47

ricardoV94 approved these changes Dec 9, 2022

View reviewed changes

ferrine force-pushed the graph-replace branch from 17e189f to 42b403a Compare December 9, 2022 16:56

ferrine added 5 commits December 9, 2022 19:58

move clone_replace to a separate file

8e86d93

refactor is_in_ancestors to support multiple inputs

49f1898

add variable_depends_on

8d9d75e

add truncated_graph_inputs function

db551b0

add graph_replace function

7b4cb73

ferrine force-pushed the graph-replace branch from 42b403a to 7b4cb73 Compare December 9, 2022 16:58

ricardoV94 merged commit befc177 into main Dec 9, 2022

ferrine deleted the graph-replace branch December 9, 2022 21:17

ricardoV94 reviewed Dec 9, 2022

View reviewed changes

larryshamalama mentioned this pull request Dec 13, 2022

clone_replace imports yield ImportError? pymc-devs/pymc#6391

Closed

ricardoV94 mentioned this pull request Dec 13, 2022

Fix bug in truncated_graph_inputs #113

Merged

ricardoV94 mentioned this pull request Jan 10, 2023

ENH: implement graph substitution that preserves independent parts #19

Closed

		)


		def test_graph_replace():

Graph replace #66

Graph replace #66

Uh oh!

Conversation

ferrine commented Nov 30, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aseyboldt left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ferrine commented Dec 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ferrine commented Dec 1, 2022

Uh oh!

ferrine commented Dec 1, 2022

Uh oh!

Uh oh!

Uh oh!

codecov-commenter commented Dec 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

ferrine commented Dec 5, 2022

Uh oh!

Uh oh!

aseyboldt commented Dec 5, 2022

Uh oh!

ricardoV94 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ricardoV94 commented Dec 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

ricardoV94 Dec 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ferrine Dec 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ricardoV94 Dec 6, 2022

Choose a reason for hiding this comment

Uh oh!

ricardoV94 Dec 6, 2022

Choose a reason for hiding this comment

Uh oh!

ferrine Dec 6, 2022

Choose a reason for hiding this comment

Uh oh!

ricardoV94 Dec 7, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

aseyboldt commented Dec 6, 2022

Uh oh!

ferrine commented Dec 6, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ferrine commented Nov 30, 2022 •

edited

Loading

ferrine commented Dec 1, 2022 •

edited

Loading

codecov-commenter commented Dec 1, 2022 •

edited

Loading

ricardoV94 commented Dec 6, 2022 •

edited

Loading

ricardoV94 Dec 6, 2022 •

edited

Loading

ferrine Dec 6, 2022 •

edited

Loading

ricardoV94 left a comment •

edited

Loading

ferrine commented Dec 9, 2022 •

edited

Loading