Refactored unit tests for FCI #68

zhi-yi-huang · 2022-08-13T09:33:56Z

Updates

Fixed FCI bugs
Refactored unit tests for FCI
Fixed GeneralGraph bugs
Refactored DAG2PAG
Refactored unit tests for PC

Description

Fixed the bug that line 85-86 in the code as of commit d75b215 would cause the recursion error.
Fix the bug that the dictionary object previous in function getPossibleDsep is not updated.
The variable dpath is used to record whether there is a directed path between two variables. But the previous implementation does not determine whether the edges are fully directed or not.
In GeneralGraph's remove_edge function, not every call of function remove_edge needs to reconstitute dpath, it only has an effect on dpath when a directed edge is removed. In order to reduce unnecessary reconstruction, so I made a modification to the remove_edge function.
We added a random graph (erdos renyi graph) to the test.

Test Plan

python -m unittest tests.TestFCI # should pass

python -m unittest tests.TestPC # should pass

tofuwen

thanks for your awesome work!!! :)

tofuwen · 2022-09-04T17:36:19Z

causallearn/search/ConstraintBased/FCI.py

            if self.existsSemidirectedPath(node_r, node_x, graph) or self.existsSemidirectedPath(node_r, node_b, graph):
-                if self.existOnePathWithPossibleParents(previous, node_r, node_x, node_b, graph):
-                    return True
+                return True


It seems the code logic got changed here.....

Is it because our original implementation has bug?

Yes, this will cause "RecursionError: maximum recursion depth exceeded while calling a Python object". And I compared the code in Tetrad and it seems that these two lines of code are not needed.

tofuwen · 2022-09-04T17:38:51Z

tests/TestFCI.py

 from causallearn.utils.GraphUtils import GraphUtils
 from causallearn.utils.PCUtils.BackgroundKnowledge import BackgroundKnowledge

+sys.path.append("")


we shouldn't do this.

Could you check discussion here and make changes correspondingly?

#59

tofuwen · 2022-09-05T01:48:13Z

tests/TestFCI.py

+        ground_truth_dag.add_directed_edge(ground_truth_nodes[0], ground_truth_nodes[1])
+        ground_truth_dag.add_directed_edge(ground_truth_nodes[0], ground_truth_nodes[2])
+        ground_truth_dag.add_directed_edge(ground_truth_nodes[1], ground_truth_nodes[3])
+        ground_truth_dag.add_directed_edge(ground_truth_nodes[2], ground_truth_nodes[3])


please do a for loop here, instead of manually add every edge.

e.g.

for node_1, node2 in [(0, 1), (0, 2), (1, 3), (2, 3)]: ground_truth_dag.add_directed_edge(ground_truth_nodes[node_1], ground_truth_nodes[node_1])

tofuwen · 2022-09-05T01:48:46Z

tests/TestFCI.py

+        ground_truth_dag.add_directed_edge(ground_truth_nodes[7], ground_truth_nodes[0])
+        ground_truth_dag.add_directed_edge(ground_truth_nodes[7], ground_truth_nodes[1])
+        ground_truth_dag.add_directed_edge(ground_truth_nodes[8], ground_truth_nodes[3])
+        ground_truth_dag.add_directed_edge(ground_truth_nodes[8], ground_truth_nodes[4])
+        ground_truth_dag.add_directed_edge(ground_truth_nodes[2], ground_truth_nodes[5])
+        ground_truth_dag.add_directed_edge(ground_truth_nodes[2], ground_truth_nodes[6])
+        ground_truth_dag.add_directed_edge(ground_truth_nodes[5], ground_truth_nodes[1])
+        ground_truth_dag.add_directed_edge(ground_truth_nodes[6], ground_truth_nodes[3])
+        ground_truth_dag.add_directed_edge(ground_truth_nodes[3], ground_truth_nodes[0])
+        ground_truth_dag.add_directed_edge(ground_truth_nodes[1], ground_truth_nodes[4])


same here, maybe use for loop

tofuwen · 2022-09-05T01:49:03Z

tests/TestFCI.py

+        ground_truth_dag.add_directed_edge(ground_truth_nodes[0], ground_truth_nodes[2])
+        ground_truth_dag.add_directed_edge(ground_truth_nodes[1], ground_truth_nodes[2])
+        ground_truth_dag.add_directed_edge(ground_truth_nodes[2], ground_truth_nodes[3])
+        ground_truth_dag.add_directed_edge(ground_truth_nodes[2], ground_truth_nodes[4])


tofuwen · 2022-09-05T01:49:11Z

tests/TestFCI.py

+        ground_truth_dag.add_directed_edge(ground_truth_nodes[7], ground_truth_nodes[0])
+        ground_truth_dag.add_directed_edge(ground_truth_nodes[7], ground_truth_nodes[5])
+        ground_truth_dag.add_directed_edge(ground_truth_nodes[8], ground_truth_nodes[0])
+        ground_truth_dag.add_directed_edge(ground_truth_nodes[8], ground_truth_nodes[6])
+        ground_truth_dag.add_directed_edge(ground_truth_nodes[9], ground_truth_nodes[3])
+        ground_truth_dag.add_directed_edge(ground_truth_nodes[9], ground_truth_nodes[4])
+        ground_truth_dag.add_directed_edge(ground_truth_nodes[9], ground_truth_nodes[6])
+        ground_truth_dag.add_directed_edge(ground_truth_nodes[0], ground_truth_nodes[1])
+        ground_truth_dag.add_directed_edge(ground_truth_nodes[0], ground_truth_nodes[2])
+        ground_truth_dag.add_directed_edge(ground_truth_nodes[1], ground_truth_nodes[2])
+        ground_truth_dag.add_directed_edge(ground_truth_nodes[2], ground_truth_nodes[4])
+        ground_truth_dag.add_directed_edge(ground_truth_nodes[5], ground_truth_nodes[6])
+


tofuwen · 2022-09-05T01:50:52Z

btw, could you update your description to contain more details on the bug you fixed?

We should write a better descriptions in general (i.e. for every possible PRs) for people who check this PR later.

jdramsey · 2022-09-05T06:10:32Z

...Someone must have been concerned about cycles in the graph. Joe

…

On Mon, Sep 5, 2022 at 1:54 AM Zhiyi Huang ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In causallearn/search/ConstraintBased/FCI.py <#68 (comment)>: > @@ -82,8 +82,7 @@ def existOnePathWithPossibleParents(self, previous, node_w: Node, node_x: Node, continue if self.existsSemidirectedPath(node_r, node_x, graph) or self.existsSemidirectedPath(node_r, node_b, graph): - if self.existOnePathWithPossibleParents(previous, node_r, node_x, node_b, graph): - return True + return True Yes, this will cause "RecursionError: maximum recursion depth exceeded while calling a Python object". And I compared the code <https://github.com/cmu-phil/tetrad/blob/70b0506af635d4f8906b3e29124bb5d01343768b/tetrad-lib/src/main/java/edu/cmu/tetrad/graph/GraphUtils.java#L4768> in Tetrad and it seems that these two lines of code are not needed. — Reply to this email directly, view it on GitHub <#68 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ACLFSRYJIX6MW4YIGG7ELWLV4WDJZANCNFSM56N3OPEQ> . You are receiving this because you are subscribed to this thread.Message ID: ***@***.***>

tofuwen · 2022-09-16T02:20:49Z

Yeah, I share the same concern as Joe.

Could you please ask the owner (or whoever familiar with the code) about the context why we need
"if self.existOnePathWithPossibleParents(previous, node_r, node_x, node_b, graph):
return True")?

Someone added this previously before, so I guess very likely it's for fixing some bugs. And by removing this line, it's likely you are introducing new bugs. So we better get to know the context and figure out the solutions instead of directly deleting it.

tofuwen · 2022-09-27T21:14:22Z

@zhi-yi-huang any updates regarding this PR? :) Sorry for the late reply though hhhh

zhi-yi-huang · 2022-10-08T06:47:47Z

Sorry, I was a little busy a while ago. The latest code has been pushed to GitHub. Special thanks to @jdramsey @chenweiDelight @wean2016 for helping to fix FCI and Fas algorithm.

tofuwen · 2022-10-26T00:04:20Z

Thanks for the awesome work, @zhi-yi-huang !!!! This is great and really non-trivial, you fixed a tons of bugs that makes the package significantly better!

One final thing: since you changed GeneralGraph.py, could you run all other tests to make sure your changes doesn't break any other tests? After all tests passed, I think we are ready to push this PR.

cc @kunwuz I think we should work on enabling auto tests for all our PR. Could you do some research to see how we can enable this for our packages? :)

kunwuz · 2022-10-26T19:48:15Z

Thanks for the awesome work, @zhi-yi-huang !!!! This is great and really non-trivial, you fixed a tons of bugs that makes the package significantly better!

One final thing: since you changed GeneralGraph.py, could you run all other tests to make sure your changes doesn't break any other tests? After all tests passed, I think we are ready to push this PR.

cc @kunwuz I think we should work on enabling autotests for all our PR. Could you do some research to see how we can enable this for our packages? :)

Sure, I will explore it in the near future. Btw, since I'm not very familiar with that, any suggestions (perhaps based on previous experiences) are super welcome. For this PR, maybe we could push it after finishing some necessary tests that @zhi-yi-huang and @tofuwen think could be necessary.

tofuwen · 2022-10-26T19:57:07Z

yeah, for this PR, after @zhi-yi-huang run all other tests, we can push it.

tofuwen · 2022-11-07T18:48:49Z

@MarkDana could you help review the failed PC tests in this PR? :)

MarkDana · 2022-11-07T19:58:02Z

@tofuwen Oh thanks for reminding! Will do it later tomorrow.

MarkDana · 2022-11-09T01:29:59Z

@zhi-yi-huang Seems that this pr contains changes of the following aspects (correct me if I have misunderstandings):

Fixed FCI and Fas bugs in FCI.py and Fas.py (of which the latter will affect PC).
Graph operations in GeneralGraph.py and DAG2PAG.py (both will affect PC).
Other utils that do not affect, or unrelated to PC, e.g., DepthChoiceGenerator.py, BackgroundKnowledge.py, and TestFCI.py.
Generated FCI benchmark graphs for future testing.

The problem that causes failed tests on PC should be in 2 I guess:

Everything else same as the current main + Fas.py of this pr -> TestPC passes.
Everything else same as the current main + GeneralGraph.py of this pr -> TestPC fails.

I didn't check details into your modified graph operations, but maybe you could start from this. Also, just a kind reminder - after bugs about graphs are fixed, we need to regenerate all benchmark files (e.g., bnlearn_discrete_10000_alarm_fci_chisq_0.05.txt).

Thanks a lot 🍺!

tofuwen · 2022-12-06T19:47:13Z

@zhi-yi-huang any updates? :) Did you find the bug? Hopefully to merge this PR in the near future to make causal-learn better!

zhi-yi-huang · 2022-12-10T09:56:49Z

The problem that causes failed tests on PC should be in GeneralGraph.py. I had added TestGeneralGraph.py in branch pc and pc_with_new_generalgraph of repo. The only difference between the code on these two branches is the main difference in GenralGraph.py.

The original GenralGraph.py would cause issue #82. I have reproduced it in TestGeneralGraph.py. The root cause of this issue is the function reconstitute_dpath which lead to errors in determining ancestor nodes as shown in TestGeneralGraph.py.

After updating to the GeneralGraph.py on this commit 3cfd99e, the above issues would disappear. The result is shown in TestGeneralGraph.py.

tofuwen · 2022-12-10T15:44:27Z

@zhi-yi-huang Thanks so much for your awesome! I can definitely see you spent lots of efforts on this PR and this is definitely not a trivial task.

Just to make sure, the current PR can pass ALL of our tests, right? (NOT just PC test and your new test)

cc @MarkDana to review PC related

MarkDana · 2022-12-11T02:05:43Z

Hi @zhi-yi-huang Thanks for your efforts! Could you please commit your changes (e.g., GeneralGraph.py, TestGeneralGraph.py) in your repo to this pull request? Then I'll review it before it's finally merged. Thanks! :)

zhi-yi-huang · 2022-12-11T06:38:01Z

There are some tests that fail:

All the tests in TestANM.py fail to pass. The p_value in the assertion is inconsistent with what is expected.
The test test_skeleton_discovery in TestBackgroundKnowledge.py fail to pass. It feeds back as "AttributeError: 'str' object has no attribute 'method'". It is the ci_test method in class CausalGraph that is the cause.
The test test_pc_with_mv_fisherz_MCAR_data_assertion in TestMVPC_mv_fisherz.py fail to pass. It feeds back as "AssertionError: A test deletion fisher-z test showed no overlapping data involving variables. Please check the input data.".

These tests do not pass in the main branch code either. And it doesn't look like the changed in this PR affects these tests.

GeneralGraph.py has already updated in this PR. I'll commit the change TestGeneralGraph.py later.

kunwuz · 2023-01-15T17:43:04Z

Hi all, since this PR has already been for a while, please let me know if there is anything else before we can merge this.

MarkDana · 2023-01-15T19:02:12Z

H @kunwuz! @zhi-yi-huang identified some flaws in the original GeneralGraph class, which led to the issue in #82. Now @zhi-yi-huang has fixed them in GeneralGraph, and is regenerating all the benchmarks used for tests, as with the dependency on GeneralGraph, our original testing benchmarks might also be wrong. As soon as @zhi-yi-huang finishes this part and commits the new benchmarks, this pr is ready to go.

Thanks so much everyone (especially @zhi-yi-huang ) for your efforts!

Signed-off-by: ZhiyiHuang <huangzhiyi.chn@gmail.com>

Signed-off-by: wean2016 <weanyq@gmail.com> Signed-off-by: ZhiyiHuang <huangzhiyi.chn@gmail.com>

Signed-off-by: ZhiyiHuang <huangzhiyi.chn@gmail.com>

MarkDana · 2023-02-04T21:06:43Z

Hi @zhi-yi-huang Awesome! I just reviewed this pr and it is good to me. As an additional check, all modifications on the benchmark CPDAG results are in the form -1 -> 1 in adjacency matrices, i.e., to change some false Meeks-oriented edges back to undirected. This aligns with your observations.

@kunwuz I think this pr is ready to go!! Thanks everyone (especially @zhi-yi-huang) for your efforts!! Thanks 🍺🍺

tofuwen reviewed Sep 5, 2022

View reviewed changes

zhi-yi-huang force-pushed the main branch from edd0217 to 999df2e Compare October 8, 2022 06:40

kunwuz mentioned this pull request Feb 1, 2023

PC algorithm and Meek rules #93

Closed

zhi-yi-huang and others added 8 commits February 3, 2023 01:23

Fixed FCI bugs

2a38a47

Signed-off-by: ZhiyiHuang <huangzhiyi.chn@gmail.com>

Refactored unit tests for FCI

0ae6d6c

Signed-off-by: ZhiyiHuang <huangzhiyi.chn@gmail.com>

Refactored unit tests for FCI

f0ed760

Signed-off-by: ZhiyiHuang <huangzhiyi.chn@gmail.com>

Fixed GeneralGraph bugs

13b06a2

Signed-off-by: ZhiyiHuang <huangzhiyi.chn@gmail.com>

Refactored DAG2PAG algorithm

a6479bb

Signed-off-by: ZhiyiHuang <huangzhiyi.chn@gmail.com>

Fixed FCI and Fas bugs

5918419

Signed-off-by: wean2016 <weanyq@gmail.com> Signed-off-by: ZhiyiHuang <huangzhiyi.chn@gmail.com>

Added a random graph test

d507d77

Signed-off-by: wean2016 <weanyq@gmail.com> Signed-off-by: ZhiyiHuang <huangzhiyi.chn@gmail.com>

Added a test for GeneralGraph

ce4aa69

Signed-off-by: wean2016 <weanyq@gmail.com> Signed-off-by: ZhiyiHuang <huangzhiyi.chn@gmail.com>

zhi-yi-huang force-pushed the main branch from 8563117 to 2d6f26a Compare February 2, 2023 17:23

zhi-yi-huang force-pushed the main branch from 2d6f26a to 40ace46 Compare February 2, 2023 17:49

Refactored unit tests for PC

7fc4e5c

Signed-off-by: wean2016 <weanyq@gmail.com> Signed-off-by: ZhiyiHuang <huangzhiyi.chn@gmail.com>

zhi-yi-huang force-pushed the main branch from 40ace46 to 7fc4e5c Compare February 3, 2023 06:39

Updated

e83d6e4

Signed-off-by: ZhiyiHuang <huangzhiyi.chn@gmail.com>

kunwuz merged commit 56a410c into py-why:main Feb 4, 2023

Refactored unit tests for FCI #68

Refactored unit tests for FCI #68

Uh oh!

Conversation

zhi-yi-huang commented Aug 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Updates

Description

Test Plan

Uh oh!

tofuwen left a comment

Choose a reason for hiding this comment

Uh oh!

tofuwen Sep 4, 2022

Choose a reason for hiding this comment

Uh oh!

zhi-yi-huang Sep 5, 2022

Choose a reason for hiding this comment

Uh oh!

tofuwen Sep 4, 2022

Choose a reason for hiding this comment

Uh oh!

tofuwen Sep 5, 2022

Choose a reason for hiding this comment

Uh oh!

tofuwen Sep 5, 2022

Choose a reason for hiding this comment

Uh oh!

tofuwen Sep 5, 2022

Choose a reason for hiding this comment

Uh oh!

tofuwen Sep 5, 2022

Choose a reason for hiding this comment

Uh oh!

tofuwen commented Sep 5, 2022

Uh oh!

jdramsey commented Sep 5, 2022 via email

Uh oh!

tofuwen commented Sep 16, 2022

Uh oh!

tofuwen commented Sep 27, 2022

Uh oh!

zhi-yi-huang commented Oct 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tofuwen commented Oct 26, 2022

Uh oh!

kunwuz commented Oct 26, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tofuwen commented Oct 26, 2022

Uh oh!

tofuwen commented Nov 7, 2022

Uh oh!

MarkDana commented Nov 7, 2022

Uh oh!

MarkDana commented Nov 9, 2022

Uh oh!

tofuwen commented Dec 6, 2022

Uh oh!

zhi-yi-huang commented Dec 10, 2022

Uh oh!

tofuwen commented Dec 10, 2022

Uh oh!

MarkDana commented Dec 11, 2022

Uh oh!

zhi-yi-huang commented Dec 11, 2022

Uh oh!

kunwuz commented Jan 15, 2023

Uh oh!

MarkDana commented Jan 15, 2023

Uh oh!

MarkDana commented Feb 4, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

zhi-yi-huang commented Aug 13, 2022 •

edited

Loading

zhi-yi-huang commented Oct 8, 2022 •

edited

Loading

kunwuz commented Oct 26, 2022 •

edited

Loading