Implement uncovered circle path functionality #42

jaron-lee · 2023-02-02T17:16:04Z

Signed-off-by: Jaron Lee [email protected]

Fixes #41 by implementing an uncovered circle path function.

Changes proposed in this pull request:

Refactored uncovered_pd_paths into a protected function _uncovered_paths that exposes the original function and uncovered_circle_paths.
Added boolean argument into _uncovered_paths that changes the behavior (enforces o-o vs. o-> edges)

Before submitting

I've read and followed all steps in the Making a pull request
section of the CONTRIBUTING docs.
I've updated or added any relevant docstrings following the syntax described in the
Writing docstrings section of the CONTRIBUTING docs.
If this PR fixes a bug, I've added a test that will fail without my fix.
If this PR adds a new feature, I've added tests that sufficiently cover my new functionality.

After submitting

All GitHub Actions jobs for my pull request have passed.

Signed-off-by: Jaron Lee <[email protected]>

adam2392 · 2023-02-02T18:18:13Z

To add your name to the contributor list, just follow the pattern that others have and then add a line in _contributors.rst inside doc/whats_new/.

We should also log your contribution re the m-separation speedup for the MixedEdgeGraph. FYI I ported the class over from networkx PR because I think getting that into networkx will take quite a bit of time. In the meantime, we will maintain it in-house until all the kinks are resolved.

adam2392 · 2023-02-02T18:18:22Z

I'll review the code and logic later this weekend

Signed-off-by: Jaron Lee <[email protected]>

jaron-lee · 2023-02-02T18:25:57Z

To add your name to the contributor list, just follow the pattern that others have and then add a line in _contributors.rst inside doc/whats_new/.

I will do this.

We should also log your contribution re the m-separation speedup for the MixedEdgeGraph. FYI I ported the class over from networkx PR because I think getting that into networkx will take quite a bit of time. In the meantime, we will maintain it in-house until all the kinks are resolved.

Should I create a separate PR for that?

Signed-off-by: Jaron Lee <[email protected]>

pywhy_graphs/algorithms/pag.py

adam2392 · 2023-02-03T01:26:07Z

pywhy_graphs/algorithms/tests/test_pag.py

+    # Construct a potentially directed path
+    G = pywhy_graphs.PAG()
+    G.add_edge("A", "C", G.directed_edge_name)
+    G.add_edge("C", "A", G.circle_edge_name)
+    G.add_edges_from(
+        [("A", "u"), ("u", "x"), ("x", "y"), ("y", "z"), ("z", "C")], G.directed_edge_name
+    )
+    G.add_edge("y", "x", G.circle_edge_name)
+
+    # create a pd path from A to C through v
+    G.add_edges_from(
+        [("A", "v"), ("v", "x"), ("x", "y"), ("y", "z"), ("z", "C")], G.directed_edge_name
+    )
+    # with the bidirected edge, v,x,y is a shielded triple
+    G.add_edge("v", "y", G.bidirected_edge_name)
+
+    # check that this is asserted as not a circle path
+    _, found_uncovered_circle_path = uncovered_circle_path(G, "u", "C", 100, "A")
+    assert not found_uncovered_circle_path


Quick question regarding this section. What is the point of this section? Should it be an uncovered_pd_path, but not uncovered_circle_path?

As a sanity check I wanted make sure that if I fed in a uncovered_pd_path that it would not be detected as a uncovered_circle_path, since I was heavily depending on the code for the former and wanted to make sure that I had actually changed the right piece of the function. You'll recognize this as a copy-paste of another test which sets up an uncovered_pd_path. Made sense when I wrote it but happy to take suggestions.

adam2392 · 2023-02-03T01:26:48Z

pywhy_graphs/algorithms/pag.py

+    Typically uncovered potentially directed paths are defined by two nodes. However,
+    in its common use case within the FCI algorithm, it is usually defined relative
+    to an adjacent third node that comes before 'u'.


Suggested change

Typically uncovered potentially directed paths are defined by two nodes. However,

in its common use case within the FCI algorithm, it is usually defined relative

to an adjacent third node that comes before 'u'.

Typically uncovered circle paths are defined by two nodes. However,

in its common use case within the FCI algorithm, it is usually defined relative

to an adjacent third node that comes before 'u'.

Is this still true?

Well I thought so initially but having implemented R5 I think it is not true, so I will amend this accordingly.

adam2392

Some minor nitpicks.

Is it possible to elaborate in the Notes section of the pd_path and circle_path what is the difference relative to each other? It seems like uncovered_circle_path is a subset of the uncovered_pd_path?

jaron-lee · 2023-02-03T14:39:08Z

Some minor nitpicks.

Is it possible to elaborate in the Notes section of the pd_path and circle_path what is the difference relative to each other? It seems like uncovered_circle_path is a subset of the uncovered_pd_path?

Let me investigate further. I understood that to be true in the literature but the documentation of the existing uncovered_pd_path function didn't seem to reflect this.

In particular:

    bidirected arrows, bidirected circle arrows, or opposite arrows.
    In addition, every node beside the endpoints are unshielded,
    meaning V(i-1) and V(i+1) are not adjacent.

seems like it is excluding circle path due to no bidirected circle arrows.

Would a better refactor strategy be:

return to having only an uncovered_pd_path function
check that this function also accounts for circle paths (and amend documentation accordingly)
add an option e.g. circle_path_only so that uncovered_pd_path searches for only circle paths
?

adam2392 · 2023-02-03T14:46:40Z

Let me investigate further. I understood that to be true in the literature but the documentation of the existing uncovered_pd_path function didn't seem to reflect this.

In particular:
    bidirected arrows, bidirected circle arrows, or opposite arrows.
    In addition, every node beside the endpoints are unshielded,
    meaning V(i-1) and V(i+1) are not adjacent.
seems like it is excluding circle path due to no bidirected circle arrows.

Ah good catch. Checking the Zhang paper, I think this is a mistake: https://reader.elsevier.com/reader/sd/pii/S0004370208001008?token=B11038210A2EF8C67786AE0A3CBA24516AE2B23FF7A7F8D3E69BFC1D46F1A2D0740C966C9B241369C1B38DDEF6705E1E&originRegion=us-east-1&originCreation=20230203144418

Would a better refactor strategy be:

return to having only an uncovered_pd_path function

I'm okay with the current implementation you have, but the main thing would be to update the unit test in uncovered_pd_path to also allow bidirected circle paths.

check that this function also accounts for circle paths (and amend documentation accordingly)

+10 (good catch on this bug!)

add an option e.g. circle_path_only so that uncovered_pd_path searches for only circle paths

Sure. This is also fine w/ me. Whichever one is easier.

jaron-lee · 2023-02-03T14:48:52Z

OK, I'll fix it up and push in a bit.

Signed-off-by: Jaron Lee <[email protected]>

codecov-commenter · 2023-02-03T15:53:01Z

Codecov Report

Merging #42 (82e71b4) into main (21b4f90) will increase coverage by 0.06%.
The diff coverage is 87.50%.

@@            Coverage Diff             @@
##             main      #42      +/-   ##
==========================================
+ Coverage   68.45%   68.51%   +0.06%     
==========================================
  Files          27       27              
  Lines        2000     2007       +7     
  Branches      482      486       +4     
==========================================
+ Hits         1369     1375       +6     
  Misses        526      526              
- Partials      105      106       +1

Impacted Files	Coverage Δ
pywhy_graphs/algorithms/pag.py	`74.48% <87.50%> (+0.33%)`	⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Signed-off-by: Jaron Lee <[email protected]>

pywhy_graphs/algorithms/pag.py

docs/whats_new/v0.1.rst

Signed-off-by: Adam Li <[email protected]>

adam2392 · 2023-02-03T19:15:04Z

pywhy_graphs/algorithms/pag.py

+    forbid_node: node, optional
+        A node after 'u' which is forbidden to immediately traverse when searching for a path.


I'm not familiar w/ when you would have a forbidden node. Would you be able to perhaps add a sentence or two to the Notes section?

And perhaps a test?

adam2392

LGTM. Just a few docs touchup and a unit-test for the forbidden node case!

Signed-off-by: Jaron Lee <[email protected]>

Co-authored-by: Adam Li <[email protected]> Signed-off-by: Jaron Lee <[email protected]>

Signed-off-by: Jaron Lee <[email protected]>

jaron-lee · 2023-02-03T20:50:06Z

OK, I think I have integrated all the requested changes and all the checks are passing.

adam2392 · 2023-02-03T21:12:46Z

Thanks @jaron-lee !

jaron-lee added 5 commits February 2, 2023 12:12

implement uncovered circle path functionality

c88645a

Signed-off-by: Jaron Lee <[email protected]>

update changelog

f53b5d9

Signed-off-by: Jaron Lee <[email protected]>

fix incorrect PR number

f238f7a

Signed-off-by: Jaron Lee <[email protected]>

sort imports

7f389ba

Signed-off-by: Jaron Lee <[email protected]>

reduce line length

aa178f2

Signed-off-by: Jaron Lee <[email protected]>

adam2392 self-requested a review February 2, 2023 18:18

add name to contributors

02eca9f

Signed-off-by: Jaron Lee <[email protected]>

attempt to fix doc errors

13eb3bb

Signed-off-by: Jaron Lee <[email protected]>