-
Notifications
You must be signed in to change notification settings - Fork 8
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[ENH] Add the ability to convert a PAG to MAG #93
Conversation
Signed-off-by: Aryan Roy <aryanroy@Aryans-MacBook-Air.local>
Codecov Report
@@ Coverage Diff @@
## main #93 +/- ##
==========================================
- Coverage 77.35% 76.74% -0.61%
==========================================
Files 42 42
Lines 3413 3543 +130
Branches 984 1032 +48
==========================================
+ Hits 2640 2719 +79
- Misses 570 603 +33
- Partials 203 221 +18
... and 1 file with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
@adam2392 I can't find the |
I would just use Tetrad to draw up a few graphs; it has the nice functionality of checking if the PAG is valid (see cmu-phil/tetrad#1556). If you read the orientation rules, by breaking any of them, then you can violate the PAG. The DAGtoPAG class does not exist in dodiscover. Sorry that was just a shorthand for saying you can use the FCI algo with a graph as the Oracle CI test. |
@adam2392 converting a PAG to MAG requires implementing the Meek rules. Is there an implementation already available for it? I remember seeing something like that in some pywhy repo. |
Signed-off-by: Aryan Roy <aryanroy5678@gmail.com>
What does the PAG to MAG algo look like? |
I am following this implementation in tetrad: https://github.com/cmu-phil/tetrad/blob/c9ac3d8d900c5917b363055f5d4f04b15068229b/tetrad-lib/src/main/java/edu/cmu/tetrad/search/utils/GraphSearchUtils.java#L411 |
Does THM 2 in the paper use these? Can you maybe summarize the algorithm here or in a docstring? Idr where the meek rules come in or why. |
The tetrad implementation does exactly what THM 2 asks in step 1. The step 2 involves removing any unshielded colliders. So I just assumed Meek rules do just that. So now I have two questions:
|
You should put this into the PR description for making communication easier to reference. Apologies, but if you can help me see where the confusion lies, it will help in answering your questions. "Let H be the graph resulting from the following procedure applied to the PAG, P: (1) orient the circles on The first part doesn't require anything except changing edges in the graph?
Read up on the PC algorithm and FCI algorithm. The FCI algorithm is described in the Zhang 2008 paper. A version of the PC algorithm is described in https://arxiv.org/pdf/1302.4972.pdf.
Not sure what the question is here. |
Sure, will do this from next time.
I was wondering if there was a pre-implemented way of getting rid of unshielded colliders in pywhy-graphs? |
Signed-off-by: Aryan Roy <aryanroy5678@gmail.com>
This isn't a "standard algorithm", so not that I'm aware of. But the idea of getting rid of unshielded colliders stems from the fact that unshielded colliders provide conditional independence (CI) constraints. DAGs encode CI constraints via d-separation. However, many DAGs can have the same CI constraints. Thus the literature invented MAGs. However, many MAGs can have the same CI constraints as well. Thus the literature invented PAGs. So the algorithm doesn't specify how to do it, just that you should be able to take the Thm2 step 2 and orient randomly as long as there isn't an unshielded collider. E.g.
|
Signed-off-by: Aryan Roy <aryanroy5678@gmail.com>
Signed-off-by: Aryan Roy <aryanroy5678@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just left a short comment cuz I had some time during lunch, but lmk when you want me to properly review this.
@adam2392 When I generated a MAG - PAG pair, I go the following edges for the MAG:
Now sometimes, my implementation of the PAG to MAG function produces this exact graph, but sometimes it produces these as well:
and
I remember you said something about there being multiple right MAGs (the MAGs would have to be markov equivalent is what you said, iirc). Are these alternate graphs right? Or is my implementation wrong? |
Aren't these the same skeleton? Just not in the same order? |
The deference is hard to notice. If you look at the first result, the edge A -> B is B -> A and in the second result, A -> C is the other way, i.e C -> A. |
Ah I see. So the question is whether or not the two graphs are Markov equivalent. Zhang has a theorem stating when two MAGS are markov equivalent. You can check those three conditions easily by hand for a small graph. |
I could not find a theorem but I did find these:
and
Is this what you are talking about? If not, can you point me to the exact place where it is defined? |
Proposition 1 is the one that you can check |
Assuming that you meant proposition 2, can you explain what exactly point 3 means in the text? |
Discriminating path is a graphical property defined in Zhang I think(?) lmk. They should also provide an example. I would take a look and walk thru examples and counter examples. There is also an implementation in pywhy-graphs if that helps the intuitive understanding. |
Alright, from what I see they seem Markov equivalent. |
How many tests should I add to finish this PR? It doesn't seem like I will be able to construct test cases to hit every line in this. |
You don't have to test every line in the |
@adam2392 I have added a GH issue, is this ok to merge? |
I will review the code. Meanwhile, there's still some CI errors, so will need to resolve those. Thanks for the patience! |
The errors are for networkx : ERROR: Package 'networkx' requires a different Python: 3.9.13 not in '>=3.10' |
|
||
|
||
def test_pag_to_mag(): | ||
pag = PAG() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As before, for unit-tests, these are commonly read by other developers, so it's useful to add some comments specifying what the test is actually testing for vs other tests. You can even split this into multiple unit-test functions w/ more descriptive naming if that's easier.
For example: test_pag_to_mag_when_graph_is_confounded_and...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Few nits:
The docstring needs to get cleaned up a bit. The unit-tests could also be better documented. It's hard to parse from an outside perspective what the test is doing
I added more context to the unit tests and cleaned up the docstring a bit. Do you want me to do something more? |
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>
* 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>
Signed-off-by: Aryan Roy <aryanroy5678@gmail.com>
Signed-off-by: Aryan Roy <aryanroy5678@gmail.com>
Signed-off-by: Aryan Roy <aryanroy5678@gmail.com>
Signed-off-by: Aryan Roy <aryanroy5678@gmail.com>
Signed-off-by: Aryan Roy <aryanroy5678@gmail.com>
Signed-off-by: Aryan Roy <aryanroy5678@gmail.com>
Signed-off-by: Aryan Roy <aryanroy5678@gmail.com>
Signed-off-by: Aryan Roy <aryanroy5678@gmail.com>
Signed-off-by: Aryan Roy <aryanroy5678@gmail.com>
Co-authored-by: Adam Li <adam2392@gmail.com> Signed-off-by: Aryan Roy <50577809+aryan26roy@users.noreply.github.com>
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>
Signed-off-by: Aryan Roy <aryanroy5678@gmail.com>
Signed-off-by: Adam Li <adam2392@gmail.com>
Thanks @aryan26roy ! |
Closes #92
Changes proposed in this pull request:
Before submitting
section of the
CONTRIBUTING
docs.Writing docstrings section of the
CONTRIBUTING
docs.After submitting