MRG, MAINT: standardize the way we get channel types#7486
MRG, MAINT: standardize the way we get channel types#7486agramfort merged 10 commits intomne-tools:masterfrom
Conversation
Codecov Report
@@ Coverage Diff @@
## master #7486 +/- ##
=========================================
- Coverage 90.08% 89.88% -0.2%
=========================================
Files 453 453
Lines 82610 82609 -1
Branches 13062 13048 -14
=========================================
- Hits 74419 74257 -162
- Misses 5362 5519 +157
- Partials 2829 2833 +4 |
larsoner
left a comment
There was a problem hiding this comment.
Overall looks like a nice simplification/refactoring, just one thing I noticed that might or might not be the case
mne/channels/channels.py
Outdated
|
|
||
| Parameters | ||
| ---------- | ||
| %(picks_nostr)s |
There was a problem hiding this comment.
I don't think this should be nostr, that's usually reserved for when info is unavailable...?
There was a problem hiding this comment.
it was accurate given how the method was defined when you reviewed. Originally it only handled integer picks, and I figured YAGNI for string-based picking, since if you know the channel name or type you want to pick, you probably know its type already. But now I realize it was easy enough to add string-based picking, and also decided there's a real use case, e.g., disambiguating mag vs grad among MEG_xxxx channel names. So now that works, and I've changed picks_nostr to picks.
|
thanks so much @drammock for cleaning this up. Let me know when to merge (CIs green). Diff looks good. |
|
What about |
|
the idea is that we should avoid having 2 functions with same name doing
different things
|
|
So what does that mean then? |
|
rename get_channel_types to get_all_channel_types and
have get_channel_types as method of data containers with an info. I think...
… |
|
ok @agramfort @larsoner other than the perpetually failing Travis OSX tests, this is green and ready for review/merge |
|
If you want to roll some community service into your PR, you can take the top half dozen or so tests here and add |
larsoner
left a comment
There was a problem hiding this comment.
Other than one idea LGTM +1 for merge
| return get_channel_type_constants() | ||
|
|
||
|
|
||
| def get_channel_type_constants(): |
There was a problem hiding this comment.
Should we add this to python_reference.rst or no?
There was a problem hiding this comment.
Personally I don't see its utility for the average user... If you disagree feel free to push that addition
This whole PR is community service 😀. But my laptop is closed for the rest of the weekend, so if you want it, it won't happen until Monday |
|
just a mac os failure. Let's merge this one. @larsoner feel free to open a new one on top of it. |
This PR tries to clean up / standardize how channel types are found in the code base.
<raw/epochs/evoked>.get_channel_types()now internally use the existing_get_channel_typesfunction instead of a pared-down reimplementation of it. Accordingly, they also now have method parameterspicks=None, unique=False, only_data_chs=False.[channel_type(info, x) for x in range(info['nchans'])]to simply call the_get_channel_types(info)or_get_channel_types(info, picks). A few more complicated list comprehensions have been left alone, as they are simpler as-is than they would have been if changed to use the_get_channel_typesfunction.raw.get_channel_types()has been added to the "info data structure" tutorial, as a contrast/alternative tochannel_typefunction.