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 MI simulations #199

Merged
merged 20 commits into from
Jan 18, 2024
Merged

ENH add MI simulations #199

merged 20 commits into from
Jan 18, 2024

Conversation

PSSF23
Copy link
Member

@PSSF23 PSSF23 commented Jan 16, 2024

Add simulations that use conditional entropy to calculate MI

Copy link

codecov bot commented Jan 16, 2024

Codecov Report

Attention: 71 lines in your changes are missing coverage. Please review.

Comparison is base (06a7066) 90.18% compared to head (b7abbd9) 88.85%.
Report is 4 commits behind head on main.

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

Files Patch % Lines
sktree/datasets/hyppo.py 31.06% 71 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #199      +/-   ##
==========================================
- Coverage   90.18%   88.85%   -1.33%     
==========================================
  Files          49       49              
  Lines        4198     4335     +137     
==========================================
+ Hits         3786     3852      +66     
- Misses        412      483      +71     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

adam2392 and others added 8 commits January 17, 2024 14:04
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>
Signed-off-by: Adam Li <adam2392@gmail.com>
Signed-off-by: Adam Li <adam2392@gmail.com>
sktree/datasets/hyppo.py Outdated Show resolved Hide resolved
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.

Otw LGTM. I have no idea why the changelog check is failing, but we can bypass it. Same w/ style

@sampan501
Copy link
Member

@adam2392 mix is currently an int, and realizing now should be a float, that takes values between 0 and 1. I don't think it should be a bool.

@adam2392
Copy link
Collaborator

Good point. Wanna add that and an error check on the value of mix being in (0,1) there?

sampan501 and others added 5 commits January 17, 2024 16:08
Signed-off-by: Adam Li <adam2392@gmail.com>
Signed-off-by: Adam Li <adam2392@gmail.com>
Signed-off-by: Adam Li <adam2392@gmail.com>
"""
rng = np.random.default_rng(seed=seed)

mu_1 = np.array([1 / np.sqrt(i) for i in range(1, n_dim + 1)])
Copy link
Member Author

Choose a reason for hiding this comment

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

Aren't these mu's reversed from what we defined on board? F should be mu and G should be -mu. @sampan501 @adam2392

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 it should be fine since it is symmetrical.

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think we should add extra confusion in it.

Copy link
Member

Choose a reason for hiding this comment

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

I personally don't feel strongly either way

Copy link
Member Author

Choose a reason for hiding this comment

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

ok

PSSF23 and others added 6 commits January 18, 2024 10:39
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 merged commit 4703a82 into main Jan 18, 2024
17 of 24 checks passed
@adam2392 adam2392 deleted the mi_truth branch January 18, 2024 18:15
@adam2392
Copy link
Collaborator

Thanks @PSSF23 and @sampan501 ! Putting this in, so we can move towards some of the other tasks

If the mu0 vs mu1 ends up being confusing, feel free to propose the change in another PR.

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