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.

Sign up for GitHub

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

Jump to bottom

[ENH] Dodiscover wrapper to GIN algorithm in causal-learn #72

Closed
wants to merge 8 commits into from
Closed

[ENH] Dodiscover wrapper to GIN algorithm in causal-learn #72

wants to merge 8 commits into from

Conversation

robertness
Copy link
Collaborator

@robertness robertness commented Dec 23, 2022 •
edited
Loading

Closes #1

Changes proposed in this pull request:

  • A base class for GIN and related algorithms
  • A GIN class that more or less PC and FCI classes
  • A unit test
  • Make GIN in causal-learn an optional dependency

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.

@robertness
Copy link
Collaborator Author

I added a basic test and GIN wrapper function.
Harsha (harsha#9298 in Discord) volunteered to flesh this out.

@adam2392 can you take a look and let us know your thoughts? I think it is important that its using architecture to the constraint-based algorithms you've built so far.

@robertness robertness mentioned this pull request Dec 23, 2022
Open
@robertness robertness mentioned this pull request Jan 4, 2023
Merged
5 tasks
@adam2392 adam2392 mentioned this pull request Jan 11, 2023
Open
@robertness
feat: Adding wrapper for GIN algorithm
9e5fda6 
Adds a wrapper from GIN algorithm from causal-learn.

Signed-off-by: Robert Ness robertness@gmail.com
adam2392
adam2392 requested changes Jan 11, 2023
View reviewed changes
Copy link
Collaborator

@adam2392 adam2392 left a comment

Choose a reason for hiding this comment

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

  • Some API nitpicks
  • try using the pywhy-graphs API for conversion between causal-learn and our graph objects

The reason being dodiscover should ideally not have any "graph-related" algorithms. It should solely focus on the discovery part.

dodiscover/replearning/gin.py Outdated Show resolved Hide resolved
dodiscover/replearning/gin.py Outdated Show resolved Hide resolved
dodiscover/replearning/gin.py Outdated Show resolved Hide resolved
dodiscover/replearning/gin.py Outdated Show resolved Hide resolved
dodiscover/replearning/gin.py Outdated Show resolved Hide resolved
dodiscover/replearning/gin.py Outdated Show resolved Hide resolved
dodiscover/replearning/gin.py Outdated Show resolved Hide resolved
@robertness robertness changed the title [WIP] Dodiscover wrapper to GIN algorithm in causal-learn [ENH] Dodiscover wrapper to GIN algorithm in causal-learn Jan 12, 2023
@codecov-commenter
Copy link

codecov-commenter commented Jan 13, 2023 •
edited
Loading

Codecov Report

Merging #72 (2cee970) into main (7f50370) will increase coverage by 0.10%.
The diff coverage is 100.00%.

❗ Current head 2cee970 differs from pull request most recent head da06694. Consider uploading reports for the commit da06694 to get more accurate results

@@            Coverage Diff             @@
##             main      #72      +/-   ##
==========================================
+ Coverage   88.13%   88.24%   +0.10%     
==========================================
  Files          25       26       +1     
  Lines        1635     1650      +15     
  Branches      266      267       +1     
==========================================
+ Hits         1441     1456      +15     
  Misses        114      114              
  Partials       80       80
Impacted Files Coverage Δ
dodiscover/replearning/gin.py 100.00% <100.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

adam2392
adam2392 reviewed Jan 13, 2023
View reviewed changes
Copy link
Collaborator

@adam2392 adam2392 left a comment

Choose a reason for hiding this comment

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

Some lint failures but otw LGTM

dodiscover/replearning/gin.py Show resolved Hide resolved
dodiscover/replearning/gin.py Show resolved Hide resolved
@robertness robertness force-pushed the GIN-algorithm branch 2 times, most recently from a18c940 to 2cee970 Compare January 13, 2023 02:35
@robertness robertness enabled auto-merge (squash) January 13, 2023 02:41
robertness and others added 6 commits January 12, 2023 19:43
@robertness @adam2392
Update README.md with philosophy (#75)
6ed3dd3 
* Update README.md

Signed-off-by: Robert Osazuwa Ness <robertness@gmail.com>
Co-authored-by: Adam Li <adam2392@gmail.com>
@adam2392 @robertness
[ENH] Adding integration test with causal learn (#54)
c206bea 
* Adding integration test with causal learn for FCI and PC algorithm
* Fix CI

Signed-off-by: Adam Li <adam2392@gmail.com>
@adam2392 @robertness
[ENH] Kernel conditional discrepancy test (conditional k-sample test)…
93dade2 
… to enable Psi-FCI algorithm (#81)

* KCD test
* Refactor utils to contain kernel utility functions

Signed-off-by: Adam Li <adam2392@gmail.com>
@adam2392 @robertness
[ENH] Conditional discrepancy (k-sample) Bregman test (#82)
28e0aa6 
* KCD test for testing conditional discrepancy
* Add bregman cd test for testing conditional discrepancy

Signed-off-by: Adam Li <adam2392@gmail.com>
@adam2392 @robertness
[ENH] Add conditional mutual information test (#83)
c84ccab 
* Adding cmi test with KNN
* Fix unit test

Signed-off-by: Adam Li <adam2392@gmail.com>
@robertness @adam2392
feat: Add wrapper to GIN algorithm
b327864 
Signed-off-by: Robert Osazuwa Ness <robertness@gmail.com>
Co-Authored-By: Adam Li <adam2392@gmail.com>
@robertness
Merge branch 'main' into GIN-algorithm
da06694 
Signed-off-by: Robert Osazuwa Ness <robertness@gmail.com>
@adam2392
Copy link
Collaborator

adam2392 commented Jan 13, 2023 •
edited
Loading

I think this DCO sign off always messes up cuz it tries to rebase. Maybe it'll be easier to just start a new PR off main and copy over your few files. Not sure why it always messes up :(

You can use poetry commands in the contributing doc to then check all the style stuff.

@robertness robertness mentioned this pull request Jan 13, 2023
Merged
@robertness robertness closed this Jan 13, 2023
auto-merge was automatically disabled January 13, 2023 18:00

Pull request was closed

@robertness
Copy link
Collaborator Author

Due to rebase error, created new PR.

@adam2392 adam2392 deleted the GIN-algorithm branch July 17, 2023 18:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@adam2392 adam2392 adam2392 requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Implement GIN
3 participants
@robertness @codecov-commenter @adam2392