Skip to content

Conversation

@drammock
Copy link
Member

This PR tries to clean up / standardize how channel types are found in the code base.

  • the instance methods <raw/epochs/evoked>.get_channel_types() now internally use the existing _get_channel_types function instead of a pared-down reimplementation of it. Accordingly, they also now have method parameters picks=None, unique=False, only_data_chs=False.
  • Various places in the codebase have been switched from list comprehensions like [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_types function.
  • the instance method raw.get_channel_types() has been added to the "info data structure" tutorial, as a contrast/alternative to channel_type function.

@codecov
Copy link

codecov bot commented Mar 21, 2020

Codecov Report

Merging #7486 into master will decrease coverage by 0.19%.
The diff coverage is 88.23%.

@@            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 larsoner added this to the 0.20 milestone Mar 21, 2020
Copy link
Member

@larsoner larsoner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall looks like a nice simplification/refactoring, just one thing I noticed that might or might not be the case


Parameters
----------
%(picks_nostr)s
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this should be nostr, that's usually reserved for when info is unavailable...?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@agramfort
Copy link
Member

thanks so much @drammock for cleaning this up. Let me know when to merge (CIs green). Diff looks good.

@cbrnr
Copy link
Contributor

cbrnr commented Mar 21, 2020

What about mne.io.pick.get_channel_types? Do you still want to rename it? Can this function use mne.io.pick._get_channel_types to avoid duplication?

@agramfort
Copy link
Member

agramfort commented Mar 21, 2020 via email

@cbrnr
Copy link
Contributor

cbrnr commented Mar 21, 2020

So what does that mean then?

@drammock
Copy link
Member Author

What about mne.io.pick.get_channel_types? Do you still want to rename it?

@cbrnr yes, see #7487

@agramfort
Copy link
Member

agramfort commented Mar 21, 2020 via email

@drammock
Copy link
Member Author

ok @agramfort @larsoner other than the perpetually failing Travis OSX tests, this is green and ready for review/merge

@drammock drammock changed the title MAINT: standardize the way we get channel types MRG, MAINT: standardize the way we get channel types Mar 22, 2020
@larsoner
Copy link
Member

If you want to roll some community service into your PR, you can take the top half dozen or so tests here and add @pytest.mark.slowtest # slow-ish on Travis OSX decorators to them

Copy link
Member

@larsoner larsoner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Other than one idea LGTM +1 for merge

return get_channel_type_constants()


def get_channel_type_constants():
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we add this to python_reference.rst or no?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Personally I don't see its utility for the average user... If you disagree feel free to push that addition

@drammock
Copy link
Member Author

If you want to roll some community service into your PR, you can take the top half dozen or so tests here and add @pytest.mark.slowtest # slow-ish on Travis OSX decorators to them

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

@agramfort
Copy link
Member

just a mac os failure. Let's merge this one. @larsoner feel free to open a new one on top of it.

@agramfort agramfort merged commit 9e78506 into mne-tools:master Mar 22, 2020
@drammock drammock deleted the DRY-channel-types branch March 23, 2020 18:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants