Throw exception in fisherz test #58

zhi-yi-huang · 2022-07-14T07:22:37Z

Updated files:

causallearn/utils/cit.py: Added exception in fisherz test
tests/TestCIT.py: Added a unit tests for fisherz test

Test plan section

python -m unittest tests.TestCIT # should pass

tofuwen · 2022-07-14T09:43:38Z

causallearn/utils/cit.py

-        inv = np.linalg.inv(sub_corr_matrix)
+        try:
+            inv = np.linalg.inv(sub_corr_matrix)
+        except np.linalg.LinAlgError:


Are you sure all the np.linalg.LinAlgError will be singularity problem?

Also please make your description more informative: e.g. "data correlation matrix is singular. Cannot run fisherz test. Please check your data"

Yes, all the np.linalg.LinAlgError in the function numpy.linalg.inv is the error of the singular matrix. Please refer to https://github.com/numpy/numpy/blob/v1.23.0/numpy/linalg/linalg.py#L483-L553.

Thanks for your suggestion! I will update it.

where did you get this information?

I checked the link you gave, and it says:

Raises ------ LinAlgError If `a` is not square or inversion fails.

Does "inversion fails" guarantee "singular matrix"? I am not sure lol --- not very familiar it.

Yes, the function np.linalg.inv gets error in here. The error is singular matrix. We can guarantee that the input matrix ’a‘ is a square matrix.

awesome, this is clear. Thanks for the clear instructions.

I am curious, without your change, what would be the output if we have data with singular correlation? I think it would directly raise LinAlgError() with "Singular Matrix" as error message?

Do you mind running an example and attach the screenshot to your test plan?

Test plan section

Test fisherz under code version 5b30aad. The test code is as follows.

import unittest import numpy as np from causallearn.utils.cit import CIT class TestCIT(unittest.TestCase): def test_fisherz_singularity_problem(self): X1 = X2 = np.random.normal(size=1000) X = np.array([X1, X2]).T cit = CIT(data=X, method='fisherz') cit.fisherz(0, 1, tuple())

Yes, all the np.linalg.LinAlgError in the function numpy.linalg.inv is the error of the singular matrix. Please refer to https://github.com/numpy/numpy/blob/v1.23.0/numpy/linalg/linalg.py#L483-L553.

Thanks for your suggestion! I will update it.

I'm not sure but will np.linalg.pinv solve it?

I tried changing the inverse to pseudo-inverse and tested fisherz tests as the code below, and the image below appeared. r in the picture is the partial correlation coefficient statistic of the variables X1 and X2 in both cases. The partial correlation coefficient of the two variables in the first case is negative, which I think is wrong. I looked at the partial correlation coefficient as described on Wikipedia, and the inverse method does require the correlation coefficient matrix to be invertible.

import unittest import numpy as np from causallearn.utils.cit import CIT class TestCIT(unittest.TestCase): def test_fisherz_singularity_problem(self): np.random.seed(767) X1 = X2 = np.random.normal(size=1000) X = np.array([X1, X2]).T cit = CIT(data=X, method='fisherz') cit.fisherz(0, 1, tuple()) X1 = X1 + 0.01 * np.random.normal(size=1000) X2 = X2 + 0.01 * np.random.normal(size=1000) X = np.array([X1, X2]).T cit = CIT(data=X, method='fisherz') cit.fisherz(0, 1, tuple())

tofuwen · 2022-07-14T09:47:19Z

tests/TestCIT.py

-                assert np.isclose(gsq(data, X, Y, S, cardinalities), gsq_notoptimized(data, X, Y, S))
-                assert np.isclose(chisq(data, X, Y, S, cardinalities), chisq_notoptimized(data, X, Y, S))
-                print(f'{X};{Y}|{S} passed')
+# def test_new_old_gsq_chisq_equivalent(self):


what's the reason to remove this test?

The functions chisq_notoptimized and gsq_notoptimized have been commented out.

Yeah, why do you comment out this test?

In the main branch, the test exists: https://github.com/cmu-phil/causal-learn/blob/main/tests/TestCIT.py

The functions chisq_notoptimized and gsq_notoptimized have been commented out in causallearn/utils/cit.py. I am not sure if I want to remove this test from the code. But I need to comment out the code in order to test my code.

@zhi-yi-huang Cool, now things is clear. Sorry I didn't read your previous message clearly. For some reason I thought chisq_notoptimized() is in the this test lol

I think @MarkDana should have context. Do you mind taking a look? Did you remove chisq_notoptimized() and forget to remove the test?

Normally, in our code base, we shouldn't keep lots of commented out code --- this will make our code more crowded without providing additional value.

I would suggest if chisq_notoptimized() is removed, we can just remove all the code related to it here.

Yes the chisq_notoptimized was removed by me in the recent commit of CIT class.
It was intended to ensure the code consistency of chisq test w/ or w/o optimization.
And I would suggest to remove the related test here too. Thanks @zhi-yi-huang and @tofuwen !

tofuwen · 2022-07-14T09:50:03Z

tests/TestCIT.py

+    def test_fisherz_singularity_problem(self):
+        X1 = X2 = np.random.normal(size=1000)
+        X = np.array([X1, X2])

-def test_new_old_gsq_chisq_equivalent(self):
-    def powerset(iterable):
-        return chain.from_iterable(combinations(list(iterable), r) for r in range(len(iterable) + 1))
+        cit = CIT(data=X, method='fisherz')

-    def _unique(column):
-        return np.unique(column, return_inverse=True)[1]
+        try:
+            cit.fisherz(0, 1, tuple())
+        except ValueError:
+            print('Catch Singularity Problem')
+            return

-    data_path = "data_discrete_10.txt"
-    data = np.loadtxt(data_path, skiprows=1)
-    data = np.apply_along_axis(_unique, 0, data).astype(np.int32)
-    cardinalities = np.max(data, axis=0) + 1
+        assert False


This is not a good way to write test in this case.

I did a simple google search using keyword: "python test expect exception", and this result looks reasonable: https://stackoverflow.com/questions/129507/how-do-you-test-that-a-python-function-throws-an-exception

Maybe you can try it. (Or maybe you can even find something better! :) )

Thanks for your suggestion! I will try it.

tofuwen · 2022-07-15T06:45:04Z

Oh, please attach a screenshot of your terminal after running the test to the test plan section, e.g. #59

tofuwen · 2022-07-16T16:16:35Z

@zhi-yi-huang Thanks for your great work!

Your comments are very clear, and cleared all my doubts, well-done! :)

Two follow-ups:

do you mind running your new code using the user reported data, i.e. "[Unsolved] 2022/6/27 Two issues on PC/fisherz: math domain error and singular matrix" section in our tracking doc? https://docs.google.com/document/d/1ENPoi3nKDTK0ba1X8Gwmj-BMcXkMEkam1gGXfXSaFTM/edit?usp=sharing
could you remove the commented-out tests (and import) to make the code looks better?

zhi-yi-huang · 2022-07-17T15:35:38Z

I can't seem to reproduce his case #29. The results I reproduced are as follows.

sub_corr_matrix
[[ 1. 0.05900512 -0.53968299 -0.77947494]
[ 0.05900512 1. -0.04393224 -0.0369982 ]
[-0.53968299 -0.04393224 1. -0.10670507]
[-0.77947494 -0.0369982 -0.10670507 1. ]]

inverse of sub_corr_matrix
[[ 1.25014681e+08, -1.01095444e+00, 7.87630530e+07, 1.05850228e+08],
[-1.01095444e+00, 1.00370144e+00, -5.88321213e-01, -8.13655359e-01],
[ 7.87630530e+07, -5.88321213e-01, 4.96231209e+07, 6.66888646e+07],
[ 1.05850228e+08, -8.13655359e-01, 6.66888646e+07, 8.96236413e+07]]

sub_corr_matrix determinant 7.8788205405274e-09

His result for the inverse is as follows.

inverse of sub_corr_matrix
[[-7.25887259e+14 -2.94842405e-01 -4.57331063e+14 -6.14610472e+14]
[-3.00253318e-01 1.00370143e+00 -1.40868234e-01 -2.12238508e-01]
[-4.57331063e+14 -1.37148843e-01 -2.88132487e+14 -3.87223301e+14]
[-6.14610472e+14 -2.07321594e-01 -3.87223301e+14 -5.20392152e+14]]

sub_corr_matrix determinant -1.356916280487041e-15

No math domain error occurs.

tofuwen · 2022-07-17T17:35:58Z

@zhi-yi-huang Cool, please remove the commented code and after that, I think this PR is ready to be pushed.

tofuwen

awesome work!

good to go! cc @kunwuz

tofuwen · 2022-07-18T03:56:58Z

one small nit: in title, "expection" => "exception"

Throw expection in fisherz test

b645b52

tofuwen reviewed Jul 14, 2022

View reviewed changes

Updated fisherz test

6bff75b

Fixed fisherz test

dd1a13e

Removed the commented code

c4a8e4e

tofuwen approved these changes Jul 18, 2022

View reviewed changes

kunwuz changed the title ~~Throw expection in fisherz test~~ Throw exception in fisherz test Jul 18, 2022

kunwuz merged commit ecbd635 into py-why:main Jul 18, 2022

kunwuz mentioned this pull request Jul 22, 2022

fisherz test occasionally (but rarely) errors out #29

Closed

MarkDana mentioned this pull request Jul 22, 2022

Refactor CITs in oop way #62

Merged

kunwuz mentioned this pull request Oct 13, 2023

ValueError: math domain error in PC with missing data #138

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Throw exception in fisherz test #58

Throw exception in fisherz test #58

zhi-yi-huang commented Jul 14, 2022 •

edited by kunwuz

tofuwen Jul 14, 2022

zhi-yi-huang Jul 14, 2022

tofuwen Jul 15, 2022

zhi-yi-huang Jul 15, 2022

tofuwen Jul 15, 2022

zhi-yi-huang Jul 15, 2022

MarkDana Jul 15, 2022

zhi-yi-huang Jul 16, 2022 •

edited

tofuwen Jul 14, 2022

zhi-yi-huang Jul 14, 2022

tofuwen Jul 15, 2022

zhi-yi-huang Jul 15, 2022

tofuwen Jul 15, 2022

MarkDana Jul 15, 2022

tofuwen Jul 14, 2022

zhi-yi-huang Jul 14, 2022

tofuwen commented Jul 15, 2022

tofuwen commented Jul 16, 2022

zhi-yi-huang commented Jul 17, 2022

tofuwen commented Jul 17, 2022

tofuwen left a comment

tofuwen commented Jul 18, 2022

Throw exception in fisherz test #58

Throw exception in fisherz test #58

Conversation

zhi-yi-huang commented Jul 14, 2022 • edited by kunwuz

Updated files:

Test plan section

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

Test plan section

Choose a reason for hiding this comment

zhi-yi-huang Jul 16, 2022 • edited

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

Choose a reason for hiding this comment

tofuwen commented Jul 15, 2022

tofuwen commented Jul 16, 2022

zhi-yi-huang commented Jul 17, 2022

tofuwen commented Jul 17, 2022

tofuwen left a comment

Choose a reason for hiding this comment

tofuwen commented Jul 18, 2022

zhi-yi-huang commented Jul 14, 2022 •

edited by kunwuz

zhi-yi-huang Jul 16, 2022 •

edited