[ENH] Add the ability to convert a DAG to an MAG #96

aryan26roy · 2023-09-08T16:35:02Z

Closes #95

Changes proposed in this pull request:

function to that takes a DAG as an argument and outputs a valid MAG.

The algorithm is described in Zhang et al. 2008 on page number 1877.

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: Aryan Roy <aryanroy5678@gmail.com>

Signed-off-by: Aryan Roy <50577809+aryan26roy@users.noreply.github.com>

Signed-off-by: Aryan Roy <aryanroy5678@gmail.com>

codecov · 2023-09-08T16:46:01Z

Codecov Report

Merging #96 (6cf1edf) into main (2d6f1d3) will decrease coverage by 2.67%.
Report is 9 commits behind head on main.
The diff coverage is 60.22%.

@@            Coverage Diff             @@
##             main      #96      +/-   ##
==========================================
- Coverage   80.02%   77.35%   -2.67%     
==========================================
  Files          39       42       +3     
  Lines        2838     3413     +575     
  Branches      759      984     +225     
==========================================
+ Hits         2271     2640     +369     
- Misses        408      570     +162     
- Partials      159      203      +44

Files	Coverage Δ
pywhy_graphs/classes/augmented.py	`78.04% <100.00%> (+8.62%)`	⬆️
pywhy_graphs/functional/additive.py	`83.33% <100.00%> (+6.86%)`	⬆️
pywhy_graphs/functional/linear.py	`70.27% <90.00%> (+3.60%)`	⬆️
pywhy_graphs/viz/draw.py	`76.00% <77.77%> (-0.93%)`	⬇️
pywhy_graphs/algorithms/multidomain.py	`59.09% <22.22%> (+9.09%)`	⬆️
pywhy_graphs/functional/multidomain.py	`11.88% <7.69%> (+0.30%)`	⬆️
pywhy_graphs/algorithms/generic.py	`89.59% <92.16%> (+4.01%)`	⬆️
pywhy_graphs/functional/discrete.py	`60.37% <60.37%> (ø)`
pywhy_graphs/functional/sampling.py	`0.00% <0.00%> (ø)`
pywhy_graphs/functional/base.py	`57.44% <57.44%> (ø)`
... and 1 more

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

adam2392 · 2023-09-08T17:24:09Z

Can you rebase this? The GH files changed tab is showing a lot of files for some reason.

aryan26roy · 2023-09-09T05:35:26Z

Sure.

Signed-off-by: Aryan Roy <aryanroy5678@gmail.com>

Bumps [actions/checkout](https://github.com/actions/checkout) from 3 to 4. - [Release notes](https://github.com/actions/checkout/releases) - [Changelog](https://github.com/actions/checkout/blob/main/CHANGELOG.md) - [Commits](actions/checkout@v3...v4) --- updated-dependencies: - dependency-name: actions/checkout dependency-type: direct:production update-type: version-update:semver-major ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

Signed-off-by: Aryan Roy <aryanroy5678@gmail.com>

aryan26roy · 2023-09-09T05:47:59Z

@adam2392 I think it's fine now.

Signed-off-by: Aryan Roy <aryanroy5678@gmail.com>

aryan26roy · 2023-09-09T09:17:09Z

@adam2392 I think I figured out how to implement the algorithm. Can you look over the comments and tell me if that is correct or not?

Signed-off-by: Aryan Roy <aryanroy5678@gmail.com>

adam2392

Yeah it looks reasonably correct. Having a few tests, examples that we can refer to visually will be helpful

pywhy_graphs/algorithms/generic.py

Signed-off-by: Aryan Roy <aryanroy5678@gmail.com>

aryan26roy · 2023-09-12T10:40:40Z

Hey @adam2392 , how do I get/generate valid DAG, MAG pairs? The implementation is complete and I need to test it now.

adam2392 · 2023-09-12T11:59:59Z

Nice!

Perhaps peruse papers or come up with your own and draw them in tetrad

aryan26roy · 2023-09-13T14:31:47Z

@adam2392 I found a pair on the internet and just wanted to confirm, is this a correct DAG and MAG pair? H is a latent variable.

adam2392 · 2023-09-13T14:56:36Z

Seems right walking thru each condition of the DAG to MAG algo.

aryan26roy · 2023-09-16T01:51:29Z

I don't see how the bidirected edge from E to R came about. Isn't E present in the ancestral graph of R U S ?

adam2392 · 2023-09-16T17:41:01Z

Oh yeah sorry. It should be E -> R right?

See the definition on pg 1877 and the last paragraph on section 2.3.

Maybe it would help if you explicitly wrote the algorithm into the docstring too.

aryan26roy · 2023-09-18T13:38:40Z

@adam2392 If two nodes are adjacent in a graph, can we say an inducing path exists between the two of them given all the conditions for an inducing path are satisfied?

adam2392 · 2023-09-18T13:40:25Z

I believe the definition states this is trivially true. IIRC

Signed-off-by: Aryan Roy <aryanroy5678@gmail.com>

aryan26roy · 2023-09-18T13:54:08Z

Ah! I found a bug in my inducing paths implementation. I never check to see if either X or Y are members of L or S (which would immediately make it impossible for them to have an inducing path between them, according to what I understood.) Since this is a small thing to fix, can I just do it in this PR?

aryan26roy · 2023-09-21T13:14:14Z

@adam2392 in the MAG shouldn't A -> E be A - E? My implementation seems to be working correctly.

Signed-off-by: Aryan Roy <aryanroy5678@gmail.com>

aryan26roy · 2023-09-21T13:39:05Z

What's the reasoning?

A is an ancestor of E union S and E is an ancestor of A union S. When both are present in the ancestor sets its a undirected edge, right?

Signed-off-by: Aryan Roy <aryanroy5678@gmail.com>

aryan26roy · 2023-09-21T14:18:10Z

@adam2392 I added two more tests, one of them is from a zhang paper and the method worked on it on the first try! I think the implementation is correct. Is the number of tests ok?

Signed-off-by: Aryan Roy <aryanroy5678@gmail.com>

adam2392

Nice! I didn't evaluate the tests yet but I left some comments on the readability of the code.

pywhy_graphs/algorithms/tests/test_generic.py

pywhy_graphs/algorithms/generic.py

Signed-off-by: Aryan Roy <aryanroy5678@gmail.com>

adam2392

Okay the code is looking better.

the tests can be improved by explicitly checking for the conditions of the MAG

pywhy_graphs/algorithms/generic.py

pywhy_graphs/algorithms/tests/test_generic.py

Co-authored-by: Adam Li <adam2392@gmail.com> Signed-off-by: Aryan Roy <50577809+aryan26roy@users.noreply.github.com>

Signed-off-by: Aryan Roy <aryanroy5678@gmail.com>

pywhy_graphs/algorithms/generic.py

Signed-off-by: Aryan Roy <aryanroy5678@gmail.com>

adam2392

LGTM! Thanks @aryan26roy

Signed-off-by: Aryan Roy <aryanroy5678@gmail.com>

aryan26roy · 2023-09-26T15:28:23Z

Using frozenset was a better solution!

pywhy_graphs/algorithms/generic.py

adam2392

Actually small code-smell. Otw lgtm

pywhy_graphs/algorithms/generic.py

Signed-off-by: Aryan Roy <aryanroy5678@gmail.com>

aryan26roy · 2023-09-26T16:11:17Z

@adam2392 Incorporated all the suggestions you made.

adam2392

LGTM assuming CIs pass!

* Add is_maximal function * add DAG to MAG function --------- Signed-off-by: Aryan Roy <aryanroy5678@gmail.com> Co-authored-by: Adam Li <adam2392@gmail.com>

aryan26roy and others added 3 commits September 8, 2023 22:02

Added function signature

9221f64

Signed-off-by: Aryan Roy <aryanroy5678@gmail.com>

Merge branch 'main' into aryan_dag_to_mag

74b6612

Signed-off-by: Aryan Roy <50577809+aryan26roy@users.noreply.github.com>

fixed the return statement

03eb9cb

Signed-off-by: Aryan Roy <aryanroy5678@gmail.com>

aryan26roy and others added 5 commits September 9, 2023 11:10

Rebased onto main

65ff956

Signed-off-by: Aryan Roy <aryanroy5678@gmail.com>

testing

1e42a20

Signed-off-by: Aryan Roy <aryanroy5678@gmail.com>

Added function signature and rebased

f0f25d4

Signed-off-by: Aryan Roy <aryanroy5678@gmail.com>

Resolved merge issue

f293c99

Signed-off-by: Aryan Roy <aryanroy5678@gmail.com>

Wrote down the algorithm

5df8578

Signed-off-by: Aryan Roy <aryanroy5678@gmail.com>

Added partial implementation

7fd8c93

Signed-off-by: Aryan Roy <aryanroy5678@gmail.com>

adam2392 reviewed Sep 10, 2023

View reviewed changes

pywhy_graphs/algorithms/generic.py Outdated Show resolved Hide resolved

aryan26roy added 2 commits September 11, 2023 19:09

Added page number

e5f16bb

Signed-off-by: Aryan Roy <aryanroy5678@gmail.com>

Completed implementation

e247bd7

Signed-off-by: Aryan Roy <aryanroy5678@gmail.com>

Added some tests and improved the implementation

b8b69d2

Signed-off-by: Aryan Roy <aryanroy5678@gmail.com>

Added two working tests

f6656d4

Signed-off-by: Aryan Roy <aryanroy5678@gmail.com>

Added another test

7832e88

Signed-off-by: Aryan Roy <aryanroy5678@gmail.com>

aryan26roy added 3 commits September 21, 2023 19:48

Linting

302733a

Signed-off-by: Aryan Roy <aryanroy5678@gmail.com>

Changelog and linting

916743a

Signed-off-by: Aryan Roy <aryanroy5678@gmail.com>

Linting

4e9b7e5

Signed-off-by: Aryan Roy <aryanroy5678@gmail.com>

adam2392 requested changes Sep 21, 2023

View reviewed changes

pywhy_graphs/algorithms/tests/test_generic.py Show resolved Hide resolved

pywhy_graphs/algorithms/generic.py Outdated Show resolved Hide resolved

pywhy_graphs/algorithms/generic.py Outdated Show resolved Hide resolved

aryan26roy added 2 commits September 21, 2023 20:46

implemented reccomendations on code readabiliy

f7eb0b8

Signed-off-by: Aryan Roy <aryanroy5678@gmail.com>

Linting

7e30a76

Signed-off-by: Aryan Roy <aryanroy5678@gmail.com>

adam2392 reviewed Sep 21, 2023

View reviewed changes

pywhy_graphs/algorithms/generic.py Show resolved Hide resolved

pywhy_graphs/algorithms/generic.py Outdated Show resolved Hide resolved

pywhy_graphs/algorithms/tests/test_generic.py Show resolved Hide resolved

aryan26roy and others added 5 commits September 23, 2023 11:51

Update pywhy_graphs/algorithms/generic.py

4daf699

Co-authored-by: Adam Li <adam2392@gmail.com> Signed-off-by: Aryan Roy <50577809+aryan26roy@users.noreply.github.com>

Update pywhy_graphs/algorithms/generic.py

822e355

Co-authored-by: Adam Li <adam2392@gmail.com> Signed-off-by: Aryan Roy <50577809+aryan26roy@users.noreply.github.com>

Added adc check to each MAG

4657c72

Signed-off-by: Aryan Roy <aryanroy5678@gmail.com>

Added tests for inducing path edge case

29ac9cf

Signed-off-by: Aryan Roy <aryanroy5678@gmail.com>

Added checks for maximality in tests

f8865e6

Signed-off-by: Aryan Roy <aryanroy5678@gmail.com>

adam2392 reviewed Sep 26, 2023

View reviewed changes

pywhy_graphs/algorithms/generic.py Outdated Show resolved Hide resolved

improved memoisation

58723ed

Signed-off-by: Aryan Roy <aryanroy5678@gmail.com>

adam2392 approved these changes Sep 26, 2023

View reviewed changes

Using frozenset

d5fb5b9

Signed-off-by: Aryan Roy <aryanroy5678@gmail.com>

adam2392 reviewed Sep 26, 2023

View reviewed changes

pywhy_graphs/algorithms/generic.py Outdated Show resolved Hide resolved

adam2392 requested changes Sep 26, 2023

View reviewed changes

adam2392 reviewed Sep 26, 2023

View reviewed changes

pywhy_graphs/algorithms/generic.py Show resolved Hide resolved

adam2392 reviewed Sep 26, 2023

View reviewed changes

pywhy_graphs/algorithms/generic.py Show resolved Hide resolved

Added test for is_maximal

6cf1edf

Signed-off-by: Aryan Roy <aryanroy5678@gmail.com>

adam2392 approved these changes Sep 26, 2023

View reviewed changes

adam2392 merged commit 9f3e202 into py-why:main Sep 26, 2023
26 of 28 checks passed

aryan26roy deleted the aryan_dag_to_mag branch September 26, 2023 16:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ENH] Add the ability to convert a DAG to an MAG #96

[ENH] Add the ability to convert a DAG to an MAG #96

aryan26roy commented Sep 8, 2023

codecov bot commented Sep 8, 2023 •

edited

Loading

adam2392 commented Sep 8, 2023

aryan26roy commented Sep 9, 2023

aryan26roy commented Sep 9, 2023

aryan26roy commented Sep 9, 2023

adam2392 left a comment

aryan26roy commented Sep 12, 2023

adam2392 commented Sep 12, 2023

aryan26roy commented Sep 13, 2023

adam2392 commented Sep 13, 2023

aryan26roy commented Sep 16, 2023

adam2392 commented Sep 16, 2023

aryan26roy commented Sep 18, 2023

adam2392 commented Sep 18, 2023

aryan26roy commented Sep 18, 2023

aryan26roy commented Sep 21, 2023

aryan26roy commented Sep 21, 2023

aryan26roy commented Sep 21, 2023

adam2392 left a comment

adam2392 left a comment

adam2392 left a comment

aryan26roy commented Sep 26, 2023

adam2392 left a comment

aryan26roy commented Sep 26, 2023

adam2392 left a comment

[ENH] Add the ability to convert a DAG to an MAG #96

[ENH] Add the ability to convert a DAG to an MAG #96

Conversation

aryan26roy commented Sep 8, 2023

Before submitting

After submitting

codecov bot commented Sep 8, 2023 • edited Loading

Codecov Report

adam2392 commented Sep 8, 2023

aryan26roy commented Sep 9, 2023

aryan26roy commented Sep 9, 2023

aryan26roy commented Sep 9, 2023

adam2392 left a comment

Choose a reason for hiding this comment

aryan26roy commented Sep 12, 2023

adam2392 commented Sep 12, 2023

aryan26roy commented Sep 13, 2023

adam2392 commented Sep 13, 2023

aryan26roy commented Sep 16, 2023

adam2392 commented Sep 16, 2023

aryan26roy commented Sep 18, 2023

adam2392 commented Sep 18, 2023

aryan26roy commented Sep 18, 2023

aryan26roy commented Sep 21, 2023

aryan26roy commented Sep 21, 2023

aryan26roy commented Sep 21, 2023

adam2392 left a comment

Choose a reason for hiding this comment

adam2392 left a comment

Choose a reason for hiding this comment

adam2392 left a comment

Choose a reason for hiding this comment

aryan26roy commented Sep 26, 2023

adam2392 left a comment

Choose a reason for hiding this comment

aryan26roy commented Sep 26, 2023

adam2392 left a comment

Choose a reason for hiding this comment

codecov bot commented Sep 8, 2023 •

edited

Loading