MRG: Add nirs support to ICA topoplot #7427
Conversation
rob-luke
force-pushed
the
fnirs_ica_topomap
branch
from
d200ebe to
257a4a9
Compare
Codecov Report
@@ Coverage Diff @@
## master #7427 +/- ##
==========================================
+ Coverage 90.06% 90.14% +0.08%
==========================================
Files 454 454
Lines 82323 83352 +1029
Branches 13022 13333 +311
==========================================
+ Hits 74143 75139 +996
- Misses 5360 5380 +20
- Partials 2820 2833 +13 |
|
tell us when to look @rob-luke |
|
Tests look to be passing but codecov has decreased, so trying the test dataset that has overlapping channels to hit those lines. |
|
@larsoner and @agramfort could you please review this PR |
rob-luke
changed the title
|
Sorry I’ve broken the tests. Please don’t review. I will fix tomorrow |
|
Ok finally all green 😡 @larsoner and @agramfort could you please review. |
|
rather than branching with an if in each function that calls _merge_grad_data I would suggest to rename _merge_grad_data to _merge_ch_data and do the if branch in a single place. Doing a |
|
Thanks @agramfort. Have I understood your suggestion correctly (based on my last commit)? Not ready for another review yet (CI is still yellow), just want to know if I understood your comment correctly. |
|
almost :)
I would rename _merge_grad_data to _merge_ch_data so _merge_ch_data is now
used anywhere you were using so far _merge_grad_data
|
|
Thanks @agramfort. |
rob-luke
changed the title
|
@larsoner I can't understand what's causing these CIs to fail. Could you please give me hint. |
rob-luke
changed the title
|
Travis and Azure are having segfaults in the last few days, unfortunately for now you'll have to look at the logs to make sure your tests are passing :( |
|
Ok my tests are passing in the CI, except for what looks like a random error. And tests are passing on my PC. @agramfort and @larsoner could you please review. |
mne/evoked.py
Outdated
| @@ -312,7 +312,7 @@ def plot_image(self, picks=None, exclude='bads', unit=True, show=True, | |||
| def plot_topo(self, layout=None, layout_scale=0.945, color=None, | |||
| border='none', ylim=None, scalings=None, title=None, | |||
| proj=False, vline=[0.0], fig_background=None, | |||
| merge_grads=False, legend=True, axes=None, | |||
| merge_channels=False, legend=True, axes=None, | |||
There was a problem hiding this comment.
you cannot change public APIs like that. We need deprecation cycles.
There was a problem hiding this comment.
To be honest I would revert this one unless you need it already for NIRS
There was a problem hiding this comment.
No don't need this at all. I just misunderstood a previous comment and thought I was meant to change all occurrences of merge grad to merge channels. I'll change this back
mne/evoked.py
Outdated
| @@ -526,7 +526,7 @@ def get_peak(self, ch_type=None, tmin=None, tmax=None, | |||
| Defaults to 'abs'. | |||
| time_as_index : bool | |||
| Whether to return the time index instead of the latency in seconds. | |||
| merge_grads : bool | |||
| merge_channels : bool | |||
| If True, compute peak from merged gradiometer data. | |||
There was a problem hiding this comment.
same here. And docstrings should then be updated. To be honest I would revert this one unless you need it already for NIRS
|
sorry I meant internal calls not public API
… |
|
all good. I will leave all internal calls at merge_channels, and revert all public api to merge_grads. |
|
what is public API is any function or class that does **not** start with _
so here you're good.
|
|
Thanks @rob-luke |
Reference issue
Ongoing fNIRS improvements for #7057
What does this implement/fix?
Adds support for plotting Ica components.