-
Notifications
You must be signed in to change notification settings - Fork 11
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 support for AAHC clustering #92
Conversation
- updated pycrostates.io.fiff to write/read results - create test suite
- modified AAHC to use the previous cluster center as initialization for the first PC power iteration algorithm
…ew cluster members
Codecov Report
@@ Coverage Diff @@
## main #92 +/- ##
==========================================
+ Coverage 96.33% 96.92% +0.58%
==========================================
Files 54 56 +2
Lines 4120 5005 +885
==========================================
+ Hits 3969 4851 +882
- Misses 151 154 +3
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
Thank you very much for adding this algo! I just went once through the code quickly, it looks very good. I will try to test it quickly :) For the style, you can run Also, could you add an entry in the changelog: https://github.com/vferat/pycrostates/tree/main/docs/source/dev/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.
Hey @rkobler,
Thanks a lot for this contribution, we really appreciate it. As @mscheltienne said, the code is very clean. This a a great addition to the library.
I checked your code with some data, and only find one/two small issues. Please have a look at my review for more details.
I have one question about the argument |
Co-authored-by: Mathieu Scheltienne <[email protected]>
Co-authored-by: Mathieu Scheltienne <[email protected]>
Co-authored-by: Mathieu Scheltienne <[email protected]>
Co-authored-by: Mathieu Scheltienne <[email protected]>
wrong axis in cluster map computation - test cases for ignore_polarity True/False - removed tol parameter in pycrostates/cluster/aahc.py - revered 'bool' check in pycrostates.utils/_checks.py
- changed _repr_html to use GEV as formatted string.
- added unit tests for invalid arguments `ignore_polarity` and `normalize_input` - added test case for `normalize_input` argument
This is a valid question. I think the best motivation for including the case |
@rkobler is right. For ERP analysis we take into account polarities whereas we ignore it for spontaneous activities ( raw / epochs). I did not intend to include ERP microstates analysis in pycrostates at first. But it may be interesting. For now, I think we should keep the
In the meantime, I'll have a look at how much work is needed to implement changes to take into account polarities in the library. |
Ok, then yes +1 to include |
(removed ignore_polarity parameter) as suggested in vferat#92 (comment)
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.
Only minor comments remain, LGTM.
I especially like the tests on simulated data you added!
- moved verbose parameter to baseclass - removed ignore_polarity property (for now) - fixed some pylint warnings
Co-authored-by: Mathieu Scheltienne <[email protected]>
Co-authored-by: Mathieu Scheltienne <[email protected]>
Co-authored-by: Mathieu Scheltienne <[email protected]>
Fixed pytest.raises misue erro
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.
LGTM, thanks a lot for this amazing addition, and don't worry about the codacy workflow ;)
Good catch for |
Thx @mscheltienne for the fix ! LGTM ! @rkobler thanks a lot for the contribution! Everything went super smooth thanks to your code quality and fast responses 🥇 @all-contributors please add @rkobler for code, doc, ideas and test ! |
I couldn't determine any contributions to add, did you specify any contributions? I couldn't determine any contributions to add, did you specify any contributions? I've put up a pull request to add @rkobler! 🎉 |
Hi pycrostates team @vferat @mscheltienne ! 👋
Thank you for working on a python repository that integrates microstate analysis with MNE python.
I recently started using the repository and felt like adding support for the atomize and agglomerate hierarchical clustering (AAHC) algorithm. The implementation is inspired by the matlab code provided in the eeglab plugin Microstates v 1.2 [1].
Please, let me know if you see merit in adding this method to the package.
Best,
Reinmar
[1] https://www.thomaskoenig.ch/Download/EEGLAB_Microstates/Microstates1.2.zip