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 orientation rules 567 for Augmented FCI #194

Merged
merged 9 commits into from
Oct 8, 2024
Merged

Conversation

EvieQ01
Copy link
Contributor

@EvieQ01 EvieQ01 commented Aug 30, 2024

Add orientation rules 567 for Augmented FCI according to [1]. These rules are related to undirected edges, in order to deal with selection bias. (when there's no selection, they can be ignored)

[1] Jiji Zhang, "On the completeness of orientation rules for causal discovery in the presence of latent confounders and selection bias", 2008

if edge.get_endpoint1() == Endpoint.CIRCLE and edge.get_endpoint2() == Endpoint.CIRCLE:
return edge.get_node2()
elif node == edge.get_node2():
if edge.get_endpoint1() == Endpoint.CIRCLE or edge.get_endpoint2() == Endpoint.CIRCLE:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should the if statement condition here be an AND condition?

for node_D in b_circle_adj_nodes_set:
if graph.is_adjacent_to(node_A, node_D):
continue
path = GetUncoveredCirclePath(node_from=node_C, node_to=node_D, G=graph)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will there be multiple uncovered circle paths? Perhaps we can change return to yield in line 105 to make it a generator.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the suggestion. This part is modified accordingly.

if graph.is_adjacent_to(node_A, node_D):
continue
path = GetUncoveredCirclePath(node_from=node_C, node_to=node_D, G=graph)
if path is not None:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The edge between alpha and beta is not modified inside the if statement. Perhaps the edge between alpha and beta can be modified based on change_flag outside the if statement.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the edge between alpha and beta will only be modified "if there is an uncovered circle path p s.t. xxx", so then change_flag should still be kept inside the if statement?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I mean the edge between alpha and beta hasn't been modified. If change_flag is set to True, then there is an uncovered circle path between alpha and beta, and the edge between alpha and beta needs to be modified. Perhaps a change_flag can be set outside the for loop on line 443 to indicate that rule5 modifies graph G. And a local_change_flag can be set inside the for loop on line 446 to indicate that there is an uncovered circle path between alpha and beta, and the edge between alpha and beta can be modified according to the local_change_flag inside the for loop.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have added the part to allow orienting multiple uncovered circle paths between alpha and beta, and moved the change_flag accordingly. Now it is changed only when double tails are oriented.

Copy link
Collaborator

@zhi-yi-huang zhi-yi-huang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for adding the orientation rules for FCI. I have fixed a minor bug in rule 5 and regenerates the benchmark results for FCI.

python -m unittest tests.TestFCI # should pass

image

@kunwuz
Copy link
Collaborator

kunwuz commented Oct 8, 2024

Thanks a lot!! @EvieQ01 @zhi-yi-huang

@kunwuz kunwuz merged commit 7924253 into py-why:main Oct 8, 2024
2 checks passed
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