Refactored unit tests for FCI

[zhi-yi-huang]

Updates

• Fixed FCI bugs
• Refactored unit tests for FCI

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.

Test Plan

python -m unittest tests.TestFCI # should pass

[tofuwen]

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]

...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 https://github.com/cmu-phil/causal-learn/pull/68#discussion_r962502448:

@@ -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 https://github.com/cmu-phil/causal-learn/pull/68#discussion_r962502448, or unsubscribe https://github.com/notifications/unsubscribe-auth/ACLFSRYJIX6MW4YIGG7ELWLV4WDJZANCNFSM56N3OPEQ . You are receiving this because you are subscribed to this thread.Message ID: @.***>

[tofuwen]

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]

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

[zhi-yi-huang]

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]

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]

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]

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

[tofuwen]

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

[MarkDana]

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

[MarkDana]

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

1. Fixed FCI and Fas bugs in FCI.py and Fas.py (of which the latter will affect PC).
2. Graph operations in GeneralGraph.py and DAG2PAG.py (both will affect PC).
3. Other utils that do not affect, or unrelated to PC, e.g., DepthChoiceGenerator.py, BackgroundKnowledge.py, and TestFCI.py.
4. 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]

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

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]

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

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]

There are some tests that fail:

1. All the tests in TestANM.py fail to pass. The p_value in the assertion is inconsistent with what is expected.
2. 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.
3. 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]

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]

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!

[MarkDana]

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