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] Add conditional mutual information test #83

Merged
merged 11 commits into from
Jan 11, 2023
Merged

Conversation

adam2392
Copy link
Collaborator

@adam2392 adam2392 commented Jan 8, 2023

Signed-off-by: Adam Li adam2392@gmail.com

First part of #17

Changes proposed in this pull request:

  • Adds the kNN based CMI test for conditional independence and related unit test

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.

Signed-off-by: Adam Li <adam2392@gmail.com>
Signed-off-by: Adam Li <adam2392@gmail.com>
Signed-off-by: Adam Li <adam2392@gmail.com>
@codecov-commenter
Copy link

codecov-commenter commented Jan 9, 2023

Codecov Report

Merging #83 (5146004) into main (e305897) will increase coverage by 3.64%.
The diff coverage is 94.81%.

❗ Current head 5146004 differs from pull request most recent head 452d2ac. Consider uploading reports for the commit 452d2ac to get more accurate results

@@            Coverage Diff             @@
##             main      #83      +/-   ##
==========================================
+ Coverage   79.70%   83.35%   +3.64%     
==========================================
  Files          24       24              
  Lines        1493     1562      +69     
  Branches      246      259      +13     
==========================================
+ Hits         1190     1302     +112     
+ Misses        215      168      -47     
- Partials       88       92       +4     
Impacted Files Coverage Δ
dodiscover/ci/utils.py 78.12% <86.66%> (+14.63%) ⬆️
dodiscover/ci/cmi_test.py 95.83% <95.83%> (ø)
dodiscover/cd/bregman.py

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

Signed-off-by: Adam Li <adam2392@gmail.com>
robertness
robertness previously approved these changes Jan 11, 2023
Copy link
Collaborator

@robertness robertness left a comment

Choose a reason for hiding this comment

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

LGTM. Perhaps we should edit the design goals in the README to mention a suite of conditional independence tests that are far more robust than in most libraries.

Signed-off-by: Adam Li <adam2392@gmail.com>
Signed-off-by: Adam Li <adam2392@gmail.com>
Signed-off-by: Adam Li <adam2392@gmail.com>
Signed-off-by: Adam Li <adam2392@gmail.com>
@adam2392 adam2392 enabled auto-merge (squash) January 11, 2023 19:33
@adam2392
Copy link
Collaborator Author

@robertness can you approve this again, so this can be merged? Take a look at the new unit test file and this now works properly with and without a conditioning set. There was a bug before that broke the test when it was just non-conditional independence testing.

@adam2392 adam2392 mentioned this pull request Jan 11, 2023
Copy link
Collaborator

@robertness robertness left a comment

Choose a reason for hiding this comment

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

LGTM

@adam2392 adam2392 merged commit 11e378e into py-why:main Jan 11, 2023
@adam2392 adam2392 deleted the cmiknn branch January 11, 2023 22:38
robertness pushed a commit that referenced this pull request Jan 13, 2023
* Adding cmi test with KNN
* Fix unit test

Signed-off-by: Adam Li <adam2392@gmail.com>
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