-
Notifications
You must be signed in to change notification settings - Fork 2
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 different thresholding options to connectivity analysis #41
Conversation
connPFM/tests/test_ev.py
Outdated
@@ -39,23 +39,15 @@ def test_threshold_ets_matrix(): | |||
dum_mat[2, 1] = 3 | |||
dum_mat2 = np.zeros((3, 3)) | |||
dum_mat2[2, 1] = 3 | |||
th_dum = ev.threshold_ets_matrix(dum_mat, [2], 2) | |||
th_dum = utils.threshold_ets_matrix(dum_mat, thr=2, selected_idxs=2) | |||
assert np.all(th_dum == dum_mat2) | |||
|
|||
|
|||
def test_event_detection( |
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.
You should add the different rss options to the unittest testing so we don't lose coverage
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'm planning on it. That's why this is still not ready for a review 😝
Codecov Report
@@ Coverage Diff @@
## main #41 +/- ##
==========================================
+ Coverage 84.94% 86.13% +1.19%
==========================================
Files 14 15 +1
Lines 777 815 +38
==========================================
+ Hits 660 702 +42
+ Misses 117 113 -4
Continue to review full report at Codecov.
|
…ETS. Added test data for all thresholding options
This is ready @vinferrer |
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.
There is a few empty files that you commited that i believe are not necessary:
connPFM/connectivity/init.py
connPFM/debiasing/init.py
connPFM/deconvolution/init.py
connPFM/tests/init.py
Where you aware of them? what are they for? if they are being generated by the test, could you avoid it?
I am not finished |
That was 100% intentional. I could not run the tests without that. If you want to learn what they do, read this: https://stackoverflow.com/questions/448271/what-is-init-py-for |
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.
Nice work @eurunuela. However, could you make changes from the comments i left?
What do you think @vinferrer ? Are you happy with the changes? |
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.
Great job @eurunuela !!
Closes #40 .
Changes proposed in this pull request: