Added some unit tests for GIN #61

zhi-yi-huang · 2022-07-18T15:05:19Z

Description

The GIN algorithm returns the graph and the causal order. In this algorithm, the causal order and the causal graph correspond to each other, so only the causal order in the result is asserted.

Updates

Fixed the independence test problem in GIN
Added assertion to the previous test cases
Removed the plot function in previous test cases for GIN
Added tests to test GIN algorithm using the hsic independence test.
Updated docstring
Removed default parameter of indep_test
Refactored the test code

Test Plan

python -m unittest tests.TestGIN # should pass

tofuwen · 2022-07-19T04:49:29Z

causallearn/search/HiddenCausal/GIN/GIN.py

+    if indep_test_method == 'kci':
+        indep_test = KCI_UInd()
+    else:
+        raise NotImplementedError((f"Independent test method {indep_test_method} is not implemented."))
+


It seems previously we supported other test method?

Also, if we only support KCI, please indicate this clearly in your error message.

This requires kernel-based independence tests such as KCI, HSIC. But they are called differently, and I wonder if I need to write a function to eliminate their differences.

so will the old code support HSIC? Basically my point is that, we should never regress (i.e. previously supported functions are not supported anymore).

if the old code base only supports KCI, then it's fine

tofuwen · 2022-07-19T04:52:57Z

causallearn/search/HiddenCausal/GIN/GIN.py

    _, _, v = np.linalg.svd(cov_m)
    omega = v.T[:, -1]
-    return np.dot(omega, data[:, X].T)
+    return np.dot(data[:, X], omega.T)


why do we change this? will it return previous result transpose?

Reduce the overhead of data matrix transposition.

will this change the function return result?

The current return value is the old value transpose, right?

Sorry, omega seems to be a 1-dimensional array and transposing the matrix is an invalid operation. The output result is unchanged. np.dot operation on N-dimensional array with 1-dimensional array is in accordance with the algorithm logic. Please refer to numpy.dot

tofuwen · 2022-07-19T04:56:15Z

tests/TestGIN.py

+        L1 = np.random.uniform(-1, 1, size=sample_size)
+        L2 = np.random.uniform(1.2, 1.8) * L1 + np.random.uniform(-1, 1, size=sample_size)
+        X1 = np.random.uniform(1.2, 1.8) * L1 + 0.2 * np.random.uniform(-1, 1, size=sample_size)
+        X2 = np.random.uniform(1.2, 1.8) * L1 + 0.2 * np.random.uniform(-1, 1, size=sample_size)
+        X3 = np.random.uniform(1.2, 1.8) * L2 + 0.2 * np.random.uniform(-1, 1, size=sample_size)
+        X4 = np.random.uniform(1.2, 1.8) * L2 + 0.2 * np.random.uniform(-1, 1, size=sample_size)


Why do you change the data generation parameters?

The previous data generation parameters were set according to the paper, but generating according to the paper does not seem to be fully identifiable. So I changed the data generation parameters.

Why is the old data not fully identifiable? We can prove it not identifiable or just our algorithm fail to do so? Sorry causal n00b lol

It should be an independence test error caused by the data.

tofuwen · 2022-07-19T05:01:29Z

tests/TestGIN.py

        data = (data - np.mean(data, axis=0)) / np.std(data, axis=0)
-        g, k = GIN(data)
-        print(g, k)
+        _, k = GIN(data)


please give better naming (instead of naming it "k")

Basically you should never use not-meaningful variable name like "k" (only exception is well-known i and j for loops)

Thanks for the suggestion, I will update it later. I see that 'k' is often used in papers to indicate causal order, e.g. LiNGAM, so I just used k.

hmmm, makes sense haha. But it would still be good to name it clearly in our codebase (you can add comment to say this variable corresponds to k in paper)

tofuwen · 2022-07-19T05:02:12Z

tests/TestGIN.py

        data = (data - np.mean(data, axis=0)) / np.std(data, axis=0)
-        g, k = GIN(data)
-        print(g, k)
+        _, k = GIN(data)


tofuwen · 2022-07-19T05:03:14Z

tests/TestGIN.py

-        g, k = GIN(data)
-        print(g, k)
+        _, k = GIN(data)
+        k = [sorted(k_i) for k_i in k]


previously you use i, here you use k_i.

It would be better if you give consistent naming

Because 'i' means index, so change the i to k_i, I forgot to unify it when I changed it before.

got it --- please make them consistent

zhi-yi-huang · 2022-07-19T13:08:12Z

Updates

Added tests to test GIN algorithm using the hsic independence test.
Updated docstring
Removed default parameter of indep_test
Fixed tests

Test Plan

python -m unittest tests.TestGIN # should pass

tofuwen

Thanks for the great work!

Almost done --- we should do a small refactor to make code more concise.
Remember the principle: never copy and paste and re-use code whenever possible. :)

tofuwen · 2022-07-21T06:27:14Z

tests/TestGIN.py

+        ground_truth = [[0, 1], [2, 3]]
+        assert len(causal_order) == len(ground_truth)
+        for i in range(len(causal_order)):
+            assert np.isclose(causal_order[i], ground_truth[i]).all()


you don't need to use np.isclose() right?

causal_order must equal to ground_truth, right?

For the first one, I don't understand. For the second one, yes.

tofuwen · 2022-07-21T06:29:23Z

tests/TestGIN.py

+        for i in range(len(causal_order)):
+            assert np.isclose(causal_order[i], ground_truth[i]).all()

+    def test_case1_hsic(self):


this function looks almost exactly the same as last function.

Please consider re-use the code, instead of copy and paste (basically you should never copy and paste code)

You can refer to #59 to make your test clearer.

Thank you for your suggestion, I will refer to modify the test code.

tofuwen · 2022-07-21T06:31:13Z

tests/TestGIN.py

+        for i in range(len(causal_order)):
+            assert np.isclose(causal_order[i], ground_truth[i]).all()

+    def test_case2_kci(self):


same for test case 2.

Basically you can test both kci and hsic in a single function, and the only difference seems to be GIN(data, indep_test_method) indep_test_method this parameter.

Check #59

tofuwen · 2022-07-21T06:31:21Z

tests/TestGIN.py

+            assert np.isclose(causal_order[i], ground_truth[i]).all()
+
+
+    def test_case3_kci(self):


tofuwen · 2022-07-21T06:34:05Z

BTW, I really like the three tests you designed. I think that's great. :)

Also a small nit: when you made later changes, you can update your description and test plan instead of commenting a new one --- people will generally read the first one instead of scroll down to check all the conversions. This makes this PR clearer for other people. :)

tofuwen · 2022-07-22T09:23:01Z

@zhi-yi-huang do you mind addressing the comments? After that, I think we can merge this PR --- very close now

zhi-yi-huang · 2022-07-22T15:55:05Z

Sorry, I missed the email from GitHub.

tofuwen

Thanks for the great work, it looks much better now!

Finally one small thing need to be addressed

tofuwen · 2022-07-25T06:32:46Z

tests/TestGIN.py

+    def validate_result(ground_truth, estimated_result):
+        assert len(ground_truth) == len(estimated_result)
+        for i in range(len(estimated_result)):
+            assert np.isclose(estimated_result[i], ground_truth[i]).all()


why do we use "isclose()" instead of "==" here?

With integer comparison, you should expect "==", right?

I think "isclose()" is meant to compare floating point

tofuwen · 2022-07-25T07:01:09Z

This is awesome! I think this PR is ready to be merged. :)

cc @kunwuz

Added some unit tests for GIN

7cb0a45

tofuwen reviewed Jul 19, 2022

View reviewed changes

zhi-yi-huang and others added 3 commits July 19, 2022 14:46

Merge branch 'cmu-phil:main' into main

fe29ec3

Fixed GIN

948a59c

Updated GIN

d2d4381

tofuwen reviewed Jul 21, 2022

View reviewed changes

Refactored the test code

c6d1dfa

tofuwen reviewed Jul 25, 2022

View reviewed changes

Updated TestGIN.py

8708ca8

kunwuz merged commit b7bd990 into py-why:main Jul 25, 2022

		assert np.isclose(causal_order[i], ground_truth[i]).all()


		def test_case3_kci(self):

Added some unit tests for GIN #61

Added some unit tests for GIN #61

Uh oh!

Conversation

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

Uh oh!

Description

Updates

Test Plan

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zhi-yi-huang commented Jul 19, 2022

Updates

Test Plan

Uh oh!

tofuwen left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tofuwen commented Jul 21, 2022

Uh oh!

tofuwen commented Jul 22, 2022

Uh oh!

zhi-yi-huang commented Jul 22, 2022

Uh oh!

tofuwen left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tofuwen commented Jul 25, 2022

Uh oh!

zhi-yi-huang commented Jul 18, 2022 •

edited

Loading

zhi-yi-huang Jul 19, 2022 •

edited

Loading