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

[ENH] Implement the Generalised Complete Adjustment Criterion #1148

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

aryan26roy
Copy link

Implement the functionality to find the adjustment criterion for all common causal graphs: DAGs, PAGs, CPDAGs and MAGs.

Signed-off-by: Aryan Roy <aryanroy5678@gmail.com>
@aryan26roy
Copy link
Author

aryan26roy commented Mar 3, 2024

@adam2392 , I am writing a working to-do list here, let me know if you want to change the granularity or simply add some more things to it:

  • Implement a few unit tests that return adjustable vs not
  • Implement the sketch of the algorithm in dowhy
  • Patch any necessary graph search algorithms into pywhy-graphs
  • Implement a short example in the docs demonstrating usage
  • Merge PR and have pywhy-graphs be an optional dependency.

@aryan26roy
Copy link
Author

@adam2392 I was looking at the current implementation of backdoor criterion in dowhy, and it has a very weird way of handling the graphs. Should I just define the standard pywhy-graphs graph in the test and complete the unit tests part of this PR?

@adam2392
Copy link
Contributor

adam2392 commented Mar 3, 2024

Yes. I think the initial work can just use pywhy-graphs. We can revisit how to refactor if necessary once we get a working prototype.

Signed-off-by: Aryan Roy <aryanroy5678@gmail.com>
Signed-off-by: Aryan Roy <aryanroy5678@gmail.com>
@aryan26roy
Copy link
Author

@adam2392 Is this an ok number of unit tests?

@adam2392
Copy link
Contributor

adam2392 commented Mar 6, 2024

@adam2392 Is this an ok number of unit tests?

Yes to start.

I didn't take a look at the details to see whether each graph is correct or not.

It would be good to organize them semantically if possible. Graph code is inherently hard to read so usually it's nice to have good documentation for cross developer communication. Some ideas for segmenting them out: DAG, Cpdag, MAG, PAG.

@aryan26roy
Copy link
Author

@adam2392 sorry for the delay. Work was hectic.
You're right, I can segment them out easily.

Signed-off-by: Aryan Roy <aryanroy5678@gmail.com>
@aryan26roy
Copy link
Author

@adam2392 How do you want to implement the sketch in dowhy?

@adam2392
Copy link
Contributor

No problem. I've been backed up

I think a good approach is to define an API similar to whatever is in dowhy currently for any type of identification.

Then let's do some error checks to make sure input is as expected. And then we simply need to implement the various steps of the algo. Proposed in the paper. Let's assume for now each graphical step has a counterpart implemented in pywhy-graphs.

Signed-off-by: Aryan Roy <aryanroy5678@gmail.com>
@aryan26roy
Copy link
Author

@adam2392 I have completed the skeleton. Made some heavy assumptions in the LLD. Would be great if you can take a look.

@amit-sharma
Copy link
Member

@aryan26roy is this still an active PR? If so, can you resolve the conversations and share the updated PR?

@aryan26roy
Copy link
Author

@amit-sharma this is still active! Although the work has been slow. Will share the updated PR soon.

Signed-off-by: Aryan Roy <aryanroy5678@gmail.com>
Signed-off-by: Aryan Roy <aryanroy5678@gmail.com>
@amit-sharma
Copy link
Member

great to hear @aryan26roy Do post a message here once the PR is ready for review

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.

None yet

3 participants