-
Notifications
You must be signed in to change notification settings - Fork 34
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 time.py due to error in spectral_connectivity_time #98
Conversation
Thanks for the PR and spotting this issue @SezanMert ! Can you add a changelog entry to the And are you up to write a short unit-test that broke on the old version before this change, but works now? |
@@ -6,3 +6,4 @@ | |||
.. _Alex Rockhill: https://github.com/alexrockhill | |||
.. _Szonja Weigl: https://github.com/weiglszonja | |||
.. _Kenji Marshall: https://github.com/kenjimarshall | |||
.. _Sezan Mert: https://github.com/SezanMert |
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.
@SezanMert is this correct?
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.
@adam2392 Yes! Thank you!
@adam2392 FYI the AZP failure here is a GitHub problem relating to the testing-data repo. |
I'm going to just merge as is then for now... |
Thanks @SezanMert for the in depth issue and easy fix! |
* Update time.py * Fixed unit test Co-authored-by: Adam Li <adam2392@gmail.com>
PR Description
It is the prospective solution of #95
It fixes the incompatibility of dimensions of frequencies in the creation of EpochSpectroTemporalConnectivity object on line 223 by providing the frequencies of interest into the object, rather than the frequencies used in the time-frequency decomposition.
Merge Checklist
Maintainer, please confirm the following before merging: