Skip to content
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

Add two variants of the KCI test #202

Merged
merged 16 commits into from
Nov 5, 2024
Merged

Conversation

OliverSchacht
Copy link
Contributor

This PR is updating #201 so it should be easier to merge.

Original Text:
I am adding two variants of the kernel-based conditional independence test, that are aiming at improved computational efficiency.

Namely:

I also added two easy unit tests checking the implementation.
I added the algorithms to the cit class, so they can be used with the PC algorithm for causal discovery.

Happy to hear your feedback!

Signed-off-by: Oliver Schacht <[email protected]>
Signed-off-by: Oliver Schacht <[email protected]>
Signed-off-by: Oliver Schacht <[email protected]>
Signed-off-by: Oliver Schacht <[email protected]>
Signed-off-by: Oliver Schacht <[email protected]>
Signed-off-by: Oliver Schacht <[email protected]>
Signed-off-by: Oliver Schacht <[email protected]>
Signed-off-by: Oliver Schacht <[email protected]>
Signed-off-by: Oliver Schacht <[email protected]>
Signed-off-by: Oliver Schacht <[email protected]>
Signed-off-by: Oliver Schacht <[email protected]>
Signed-off-by: Oliver Schacht <[email protected]>
Signed-off-by: Oliver Schacht <[email protected]>
Signed-off-by: Oliver Schacht <[email protected]>
Signed-off-by: Oliver Schacht <[email protected]>
@MarkDana
Copy link
Collaborator

Hi @OliverSchacht thanks for your amazing contribution! I will review this PR within next week.

@kunwuz kunwuz requested a review from MarkDana October 25, 2024 16:55
@MarkDana
Copy link
Collaborator

MarkDana commented Nov 5, 2024

Dear @OliverSchacht I really appreciate this contribution!

I reviewed the codes and the logic seems good to me. I didn't identify major running issues. The two unittests passed from my end, though I didn't do other accuracy benchmarks (e.g, those in TestPC.py/test_pc_simulate_linear_nongaussian_with_kci).

(One minor point is that I see KCI.py is changed on null_samples -> self.null_samples. This change seems unrelated. Though trivial, I revert it just for sanity check. Two unittests still passed after this reversion. But please correct me if i'm wrong.)

For now I think this pr is ready to be merged. One suggestion: could you please also add documents to https://github.com/py-why/causal-learn/tree/main/docs/source/independence_tests_index, with general instructions on using FastKCI and RCIT, with parameter choices on e.g., K, J. This could be done maybe after the working paper is finished.

Thank you again :)

@MarkDana MarkDana merged commit f6aa500 into py-why:main Nov 5, 2024
2 checks passed
@kunwuz
Copy link
Collaborator

kunwuz commented Nov 26, 2024

Hi @OliverSchacht , thanks so much for your contribution! When you think it is ready, could you please write a short document as @MarkDana suggested? This would be very helpful for our users to start using this excellent test.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants