-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
Apply hilbert stc #12323
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
Apply hilbert stc #12323
Conversation
…ilbert to work on SourceEstimate object
for more information, see https://pre-commit.ci
|
working to fix the failed tests |
|
All tests passed. |
larsoner
left a 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.
Looks like a good start! See some inline comments.
Any new functionality should be accompanied by unit tests, so in mne/tests/test_source_estimate.py we'll need some basic tests of the new functions that have been added for at least the SourceEstimate class.
Co-authored-by: Eric Larson <larson.eric.d@gmail.com>
Co-authored-by: Eric Larson <larson.eric.d@gmail.com>
Co-authored-by: Eric Larson <larson.eric.d@gmail.com>
Co-authored-by: Eric Larson <larson.eric.d@gmail.com>
for more information, see https://pre-commit.ci
Co-authored-by: Eric Larson <larson.eric.d@gmail.com>
for more information, see https://pre-commit.ci
|
I have addressed all of the suggestions. I also added one function to test to |
larsoner
left a 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.
For the object.apply_hilbert function, the test_evoked.py has the test function for raw, epochs and evoked objects, I wonder if the tests should be added in the same function.
Sure!
Should we use for example raw data to generate sources? or is there a way to get access for stc_test_data ?
I would just use np.random.default_rng(0).normal(...) like here:
mne-python/mne/tests/test_source_estimate.py
Lines 373 to 379 in e6b49ea
| def _fake_stc(n_time=10, is_complex=False): | |
| np.random.seed(7) | |
| verts = [np.arange(10), np.arange(90)] | |
| data = np.random.rand(100, n_time) | |
| if is_complex: | |
| data.astype(complex) | |
| return SourceEstimate(data, verts, 0, 1e-1, "foo") |
But don't use np.random.seed, that's bad practice -- use class-based methods after instantiating a default_rng instead (modern best practice for NumPy random that we haven't fully adopted yet but should at some point!).
Co-authored-by: Eric Larson <larson.eric.d@gmail.com>
for more information, see https://pre-commit.ci
Co-authored-by: Eric Larson <larson.eric.d@gmail.com>
Co-authored-by: Eric Larson <larson.eric.d@gmail.com>
for more information, see https://pre-commit.ci
larsoner
left a 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.
Just three minor tweaks, marking for merge-when-green, thanks in advance @BabaSanfour !
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Co-authored-by: Eric Larson <larson.eric.d@gmail.com>
Fixes #12310.
Updating mne.filter.MixinFilter methods to work with Source Estimated data