dodiscover icon indicating copy to clipboard operation
dodiscover copied to clipboard

Add GranDAG algorithm

Open eeulig opened this issue 1 year ago • 4 comments

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

  • [x] I've read and followed all steps in the Making a pull request section of the CONTRIBUTING docs.
  • [x] 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.
  • [x] 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.

eeulig avatar Sep 01 '23 07:09 eeulig

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?

adam2392 avatar Sep 01 '23 21:09 adam2392

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?

eeulig avatar Sep 08 '23 16:09 eeulig

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

robertness avatar Sep 08 '23 16:09 robertness

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.

adam2392 avatar Sep 08 '23 16:09 adam2392