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 GranDAG algorithm #144

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

Add GranDAG algorithm #144

wants to merge 3 commits into from

Conversation

eeulig
Copy link

@eeulig eeulig commented Sep 1, 2023

Changes proposed in this pull request:

This PR adds the following causal discovery method: Gradient-Based Neural DAG Learning, Lachapelle et al., 2020.

The code is heavily based on the author's implementation available on GitHub (licensed under MIT license). I noted this in the docstring of the main class and added a remark for each function directly taken from above repository.

I also compared the SHD on ER graphs to the results reported in the paper (Tab. 1, 2, 8, 9) for the first 10 datasets provided by the authors here:

ER-10-1  ER-10-4 ER-20-1  ER-20-4 ER-50-1 ER-50-4
Paper 1.7 $\pm$ 2.5 8.3 $\pm$ 2.8 4.0 $\pm$ 3.4 45.2 $\pm$ 10.7 5.1 $\pm$ 2.8 102.6 $\pm$ 21.2
DoDiscover 1.6 $\pm$ 1.6 15.3 $\pm$ 2.7 2.8 $\pm$ 1.6 36.9 $\pm$ 8.5 7.1 $\pm$ 4.6 99.5 $\pm$ 19.1

Overall, results seem to be consistent, and I assume most differences can be attributed to differences in the different dataset samples.

Before submitting

  • I've read and followed all steps in the Making a pull request
    section of the CONTRIBUTING docs.
  • I've updated or added any relevant docstrings following the syntax described in the
    Writing docstrings section of the CONTRIBUTING docs.
  • If this PR fixes a bug, I've added a test that will fail without my fix.
  • If this PR adds a new feature, I've added tests that sufficiently cover my new functionality.

After submitting

  • All GitHub Actions jobs for my pull request have passed.

@adam2392
Copy link
Collaborator

adam2392 commented Sep 1, 2023

Hi @eeulig wow thanks for this PR that covers an important part of causal discovery!

This is as I understand a non-linear extension of NoTears right? This might go through a number of iterations to get the code to a mergable state. This is mainly for maintainability, code robustness and understanding. For example see the PR for topological methods #129. If you're new to contributing to open source, this will also be a great learning experience in improving your scientific coding chops! Welcome to the community :)

I'll take some time over the next week or so to review it. In the meantime, WDYT about adding an example or a few examples that demonstrate how to use these methods and when these methods are useful? These are useful to guide users that are not familiar with the methods (e.g. me too :))

You can look at the existing examples/ directory for the format and high level layout.

Summary of what we'll work towards:

  1. have a consistent API: You can follow the same API as the topological methods, or FCI. E.g. basically implement learn_graph(data, context). Most other methods can usually be private unless used elsewhere.
  2. have unit tests that sufficiently cover the relevant use-cases: I see you have added unit tests, so I will review these to see if there's anything missing
  3. have some documentation to explain to users how to use the feature: The best is an example like we have here

Lmk if this sounds good to you?

@@ -49,6 +49,7 @@ importlib-resources = { version = "*", python = "<3.10" }
pywhy-graphs = { git = "https://github.com/py-why/pywhy-graphs.git", branch = 'main', optional = true }
pygraphviz = { version = "^1.11", optional = true }
pygam = "^0.9.0"
torch = "^2.0.1"
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we are happy to have a torch dependency

return expm_input.t() * grad_output


def is_acyclic(adjacency: torch.Tensor) -> bool:
Copy link
Collaborator

Choose a reason for hiding this comment

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

@adam2392 maybe the acyclicity check on adjacency matrix should go to pywhy-graph?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes this is already implemented there

@eeulig
Copy link
Author

eeulig commented Sep 8, 2023

Thanks a lot @adam2392 and @robertness ! I'm happy to look into providing an example similar to the ones in dodiscover/examples with some further explanations!

I noticed that some checks were not successful, because some gpu related libraries (libcudnn.so, libcublas.so) could not be loaded. I think this might be because poetry installs a gpu version of torch instead of the cpu version in the test environment. Do you think this is an issue and we should enforce the cpu version of torch instead?

@robertness
Copy link
Collaborator

Is this something that could be solved with code that sets the device to cpu unless cuda is available?

@adam2392
Copy link
Collaborator

adam2392 commented Sep 8, 2023

Thanks a lot @adam2392 and @robertness ! I'm happy to look into providing an example similar to the ones in dodiscover/examples with some further explanations!

Great! I will try to find some time next week to start taking a look at this. Since this is an "extension" of notears, I want to see if it is possible to design the code in such a way to allow notears to be implemented modularly in a future PR.

I noticed that some checks were not successful, because some gpu related libraries (libcudnn.so, libcublas.so) could not be loaded. I think this might be because poetry installs a gpu version of torch instead of the cpu version in the test environment. Do you think this is an issue and we should enforce the cpu version of torch instead?

Yeah I think testing GPU on CIs is notoroiously difficult, so we can force the CPU version and the tests and examples should be super small-scale for the sake of just allowing it to run.

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