[ENH, MAINT] Refactor directed-undirected graph class #72

jaron-lee · 2023-04-06T01:32:02Z

Towards #65

Changes proposed in this pull request:

Creates a new directed/undirected graph class DiUnGraph
Refactors CPDAG to be a subclass of DiUnGraph
Create a new graph class Chain Graph which subclasses DiUnGraph
Implement functions that check if CPDAG and ChainGraph are valid instances

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]>

robertness · 2023-04-06T21:59:11Z

pywhy_graphs/classes/diungraph.py

+    of the sample induces association between ``A`` and ``B``) [1]_.
+
+
+    The implementation supports representation of both Lauritzen-Wermuth-Frydenberg (LWF)


I like this. Maybe citations?

robertness · 2023-04-06T21:59:59Z

pywhy_graphs/classes/diungraph.py

+        self,
+        incoming_directed_edges=None,
+        incoming_undirected_edges=None,
+        directed_edge_name: str = "directed",


whitespace around " = "

robertness

LGTM

robertness · 2023-04-06T22:05:22Z

I know it's still a WIP but I approved in case you wanted to get initial work in.

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

adam2392 · 2023-04-16T16:02:47Z

pywhy_graphs/classes/diungraph.py

@@ -184,6 +143,90 @@ def possible_parents(self, n: Node) -> Iterator[Node]:
        """
        return self.sub_undirected_graph().neighbors(n)

+
+class CPDAG(DiUnGraph, ConservativeMixin):
+    """Completed partially directed acyclic graphs (CPDAG).


Perhaps it is worth adding a note or warning even to the classes that validity is not guaranteed. Please use the validity check if you need to rigorously check that your construction is valid?

Same in chaingraph? Maybe also in the ADMG, and PAG too?

Yeah this is a good idea. I will address just CPDAG and Chain Graph in this PR but will look to redo the other classes in another PR.

adam2392

I like the direction of these changes. It keeps us lean but also doesn't commit to one design yet.

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

codecov-commenter · 2023-04-16T21:56:21Z

Codecov Report

Merging #72 (7d69f7f) into main (c1b4107) will increase coverage by 0.25%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main      #72      +/-   ##
==========================================
+ Coverage   84.42%   84.67%   +0.25%     
==========================================
  Files          34       35       +1     
  Lines        2555     2597      +42     
  Branches      687      695       +8     
==========================================
+ Hits         2157     2199      +42     
  Misses        251      251              
  Partials      147      147

Impacted Files	Coverage Δ
pywhy_graphs/algorithms/cg.py	`100.00% <100.00%> (ø)`
pywhy_graphs/classes/diungraph.py	`85.45% <100.00%> (ø)`

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

adam2392

Some minor fixes and then LGTM.

adam2392 · 2023-04-16T23:48:15Z

docs/whats_new/v0.1.rst

@@ -25,6 +25,7 @@ Version 0.1

 Changelog
 ---------
+- |Feature| Introduce chain graphs and validity checks, and refactor CPDAG and chain graphs to use a directed-undirected private class, by `Jaron Lee`_ (:pr:`73`)


Typically change log goes on the bottom. I think the Changelog CI check should get fixed that way

oh fair enough I was just following https://keepachangelog.com/en/1.0.0/ which has everything in reverse chronological order

adam2392 · 2023-04-16T23:48:42Z

pywhy_graphs/algorithms/cg.py

+
+    # Search over all nodes.
+    for v in all_nodes:
+        queue = deque([])


Add a type annotation to queue to fix the mypy complaint

Can do - also I have the pre-commit hooks PR which is open for review which will help me catch this :)))

adam2392 · 2023-04-16T23:48:47Z

pywhy_graphs/algorithms/cg.py

+        # Fill queue with paths from v starting with outgoing directed edge
+        # OrderedDict used for O(1) set membership and ordering
+        for _, z in G_directed.out_edges(nbunch=v):
+            d = OrderedDict()


Add a type annotation to d to fix the mypy complaint

adam2392 · 2023-04-16T23:49:47Z

pywhy_graphs/algorithms/tests/test_cg.py

+
+
+@pytest.fixture
+def fig_g1_frydenberg():


Perhaps add a docstring and a link to the reference for all the fixtures that come from some paper, so its back traceable?

yes that's a good idea, i have the reference in the function but not the test

jaron-lee · 2023-04-17T00:53:56Z

Some minor fixes and then LGTM.

I will also implement the corresponding changes for CPDAG before I unmark as draft but thanks for the early reviews on this PR

adam2392 · 2023-06-28T18:04:15Z

@jaron-lee hey know you're prolly busy doing more interesting things at your internship :)

Just wanted to check in to see if you think we should just do a release v0.1 first for now. I think we should def refactor by v0.2 into whatever this PR manifests?

jaron-lee · 2023-06-28T22:01:39Z

Yes I think going ahead with v0.1 is a good idea. I should hopefully have some more time at the end of summer - I may just reduce the scope of the refactor and let others work on it in the meantime so I don't hold anything up.

…

On Wed, Jun 28, 2023, 2:04 PM Adam Li ***@***.***> wrote: @jaron-lee <https://github.com/jaron-lee> hey know you're prolly busy doing more interesting things at your internship :) Just wanted to check in to see if you think we should just do a release v0.1 first for now. I think we should def refactor by v0.2 into whatever this PR manifests? — Reply to this email directly, view it on GitHub <#72 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAEE2MJLRSC42JHJ4TDPNLTXNRW2TANCNFSM6AAAAAAWUY2CPY> . You are receiving this because you were mentioned.Message ID: ***@***.***>

jaron-lee added 3 commits April 5, 2023 20:40

add diungraph

7a57e01

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

fix failing test

328a656

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

template for is_valid functions

1d3e990

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

jaron-lee changed the title ~~Refactor classes~~ Refactor directed-undirected graph class Apr 6, 2023

robertness reviewed Apr 6, 2023

View reviewed changes

robertness approved these changes Apr 6, 2023

View reviewed changes

add framework for chain graph function and tests

7800aee

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

adam2392 reviewed Apr 16, 2023

View reviewed changes

jaron-lee added 3 commits April 16, 2023 16:15

fix chain graph validity function

d7f71b1

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

update changelog

79f7457

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

fix spelling errors

7d69f7f

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

adam2392 reviewed Apr 16, 2023

View reviewed changes

jaron-lee changed the title ~~Refactor directed-undirected graph class~~ [ENH, MAINT] Refactor directed-undirected graph class Apr 20, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ENH, MAINT] Refactor directed-undirected graph class #72

[ENH, MAINT] Refactor directed-undirected graph class #72

jaron-lee commented Apr 6, 2023 •

edited

Loading

robertness Apr 6, 2023

robertness Apr 6, 2023

robertness left a comment

robertness commented Apr 6, 2023

adam2392 Apr 16, 2023

jaron-lee Apr 16, 2023

adam2392 left a comment

codecov-commenter commented Apr 16, 2023

adam2392 left a comment

adam2392 Apr 16, 2023

jaron-lee Apr 17, 2023

adam2392 Apr 16, 2023

jaron-lee Apr 17, 2023

adam2392 Apr 16, 2023

adam2392 Apr 16, 2023

jaron-lee Apr 17, 2023

jaron-lee commented Apr 17, 2023

adam2392 commented Jun 28, 2023

jaron-lee commented Jun 28, 2023 via email

		of the sample induces association between ``A`` and ``B``) [1]_.


		The implementation supports representation of both Lauritzen-Wermuth-Frydenberg (LWF)

[ENH, MAINT] Refactor directed-undirected graph class #72

Are you sure you want to change the base?

[ENH, MAINT] Refactor directed-undirected graph class #72

Conversation

jaron-lee commented Apr 6, 2023 • edited Loading

Before submitting

After submitting

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robertness left a comment

Choose a reason for hiding this comment

robertness commented Apr 6, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adam2392 left a comment

Choose a reason for hiding this comment

codecov-commenter commented Apr 16, 2023

Codecov Report

adam2392 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jaron-lee commented Apr 17, 2023

adam2392 commented Jun 28, 2023

jaron-lee commented Jun 28, 2023 via email

jaron-lee commented Apr 6, 2023 •

edited

Loading