-
Notifications
You must be signed in to change notification settings - Fork 182
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
Update GIN #87
base: main
Are you sure you want to change the base?
Update GIN #87
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
did part of review as code style examples~
#------------------------------------------------------------------------------- | ||
# Name: 模块1 | ||
# Purpose: | ||
# | ||
# Author: YY | ||
# | ||
# Created: 03/03/2021 | ||
# Copyright: (c) YY 2021 | ||
# Licence: <your licence> | ||
#------------------------------------------------------------------------------- |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should delete this, or fill it.
In the open-source code, we should maintain high code quality --- we shouldn't allow any unnecessary comments in the code.
Sorry there are lots of rules in industry code that are different from research code :(
import math | ||
from scipy.stats import chi2 | ||
|
||
def FisherTest(pvals,alph=0.01): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: comma between different variables.
You can follow Google Python codestyle when writing code: https://google.github.io/styleguide/pyguide.html
You can find automated tool like pylint in the link.
def FisherTest(pvals,alph=0.01): | |
def FisherTest(pvals, alph=0.01): | |
from scipy.stats import chi2 | ||
|
||
def FisherTest(pvals,alph=0.01): | ||
Fisher_Stat=0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
space between operand
Fisher_Stat=0 | |
Fisher_Stat = 0 | |
def FisherTest(pvals,alph=0.01): | ||
Fisher_Stat=0 | ||
L = len(pvals) | ||
for i in range(0,L): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for i in range(0,L): | |
for i in range(0, L): | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
address this?
Fisher_Stat=0 | ||
L = len(pvals) | ||
for i in range(0,L): | ||
if pvals[i] ==0: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if pvals[i] ==0: | |
if pvals[i] == 0: | |
if Fisher_pval >alph: | ||
return True,Fisher_pval | ||
else: | ||
return False,Fisher_pval |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if Fisher_pval >alph: | |
return True,Fisher_pval | |
else: | |
return False,Fisher_pval | |
return Fisher_pval > alph, Fisher_pval | |
|
||
|
||
def main(): | ||
pvals = [0.01,0.9] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pvals = [0.01,0.9] | |
pvals = [0.01, 0.9] | |
|
||
def main(): | ||
pvals = [0.01,0.9] | ||
FisherTest(pvals,0.1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FisherTest(pvals,0.1) | |
FisherTest(pvals, 0.1) | |
if pval > alpha: | ||
flag = True | ||
else: | ||
flag = False | ||
|
||
if not flag:#not false == ture ---> if false | ||
#print(X,Z,flag) | ||
return False |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if pval > alpha: | |
flag = True | |
else: | |
flag = False | |
if not flag:#not false == ture ---> if false | |
#print(X,Z,flag) | |
return False | |
if pval <= alpha: | |
return False |
if pvalue >alph: | ||
return True | ||
else: | ||
return False |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if pvalue >alph: | |
return True | |
else: | |
return False | |
return pvalue > alph | |
BTW, for every PR, we should have an informative description. Could you add your description? Here is a good example PR with great descriptions: #85 |
Updates Modified the whole GIN package to make it more readable
Notes |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks so much for your great work! This version is much better than previous version!
Added more comments.
And there are two main changes we need for this PR:
- could you write tests for GIN method to make sure GIN behaves as expected?
- could you edit your first message to include the description of this PR? We need to have great descriptions for every PR, so in the future, when we come back or if our users are checking the PR, we / they can know what is happening.
import math | ||
from scipy.stats import chi2 | ||
|
||
def FisherTest(pvals, alpha = 0.01): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
def FisherTest(pvals, alpha = 0.01): | |
def FisherTest(pvals, alpha=0.01): | |
def FisherTest(pvals,alph=0.01): | ||
Fisher_Stat=0 | ||
L = len(pvals) | ||
for i in range(0,L): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
address this?
else: | ||
TP = pvals[i] | ||
|
||
Fisher_Stat = Fisher_Stat - 2*math.log(TP) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fisher_Stat = Fisher_Stat - 2*math.log(TP) | |
Fisher_Stat = Fisher_Stat - 2 * math.log(TP) | |
|
||
Fisher_Stat = Fisher_Stat - 2*math.log(TP) | ||
|
||
Fisher_pval = 1 - chi2.cdf(Fisher_Stat, 2*L) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fisher_pval = 1 - chi2.cdf(Fisher_Stat, 2*L) | |
Fisher_pval = 1 - chi2.cdf(Fisher_Stat, 2 * L) | |
if Fisher_pval >alpha: | ||
return True, Fisher_pval | ||
else: | ||
return False, Fisher_pval |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if Fisher_pval >alpha: | |
return True, Fisher_pval | |
else: | |
return False, Fisher_pval | |
return Fisher_pval > alpha, Fisher_pval | |
def _entropy(u): | ||
"""Calculate entropy using the maximum entropy approximations.""" | ||
k1 = 79.047 | ||
k2 = 7.4129 | ||
gamma = 0.37457 | ||
return (1 + np.log(2 * np.pi)) / 2 - k1 * (np.mean(np.log(np.cosh(u))) - gamma)**2 - k2 * (np.mean(u * np.exp((-u**2) / 2)))**2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
seems equivalent to previous functions?
Re-use code whenever possible and never duplicate code
|
||
def MI_1(x1, y1): | ||
"""estimating mutual information by Non-parametric computation of entropy and mutual-information""" | ||
x=x1.reshape(len(x1),1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
x=x1.reshape(len(x1),1) | |
x = x1.reshape(len(x1), 1) | |
def MI_1(x1, y1): | ||
"""estimating mutual information by Non-parametric computation of entropy and mutual-information""" | ||
x=x1.reshape(len(x1),1) | ||
y=y1.reshape(len(y1),1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same
if pval > alpha: | ||
flag = True | ||
else: | ||
flag = False | ||
|
||
if not flag: | ||
return False | ||
|
||
return True |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I remembered I commented on this before?
if pval > alpha: | |
flag = True | |
else: | |
flag = False | |
if not flag: | |
return False | |
return True | |
if pval <= alpha: | |
return False | |
return True | |
temp = np.array(data[:, i]) | ||
mi = ID.MI_1(result, temp) | ||
MIS += mi | ||
MIS = MIS/len(Z) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
MIS = MIS/len(Z) | |
MIS = MIS / len(Z) | |
Hi all @fengxie009 @tofuwen, just would like to quickly check whether there are any updates on this PR. Please feel free to let others know if there are any questions :) |
Hi Yujia,
There are currently no updates. I will let you know if there are any
changes.
Best wishes,
Feng
Yujia Zheng ***@***.***> 于2023年1月31日周二 08:10写道:
… Hi all @fengxie009 <https://github.com/fengxie009> @tofuwen
<https://github.com/tofuwen>, just would like to quickly check whether
there are any updates on this PR. Please feel free to let others know if
there are any questions :)
—
Reply to this email directly, view it on GitHub
<#87 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AH6YBRX2AGGKIDDJIR5YJPLWVBKAJANCNFSM6AAAAAASWHV7YU>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
Hi @fengxie009, Do we have some updates here? :) We need to ensure that we have unit tests coverage for all the methods, as it's required here: #100 And we will build the CI once your unit tests are completed. :) |
Hi @fengxie009 and @tofuwen, are there any updates on this PR? I believe some quick tests to make sure that the modification is correct is enough to merge this. It seems to be around too long. |
No description provided.