Skip to content
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

RFC: get_channel_types polysemy #7487

Closed
drammock opened this issue Mar 20, 2020 · 5 comments
Closed

RFC: get_channel_types polysemy #7487

drammock opened this issue Mar 20, 2020 · 5 comments
Milestone

Comments

@drammock
Copy link
Member

I've stumbled on a not-so-nice aspect of the API when working on #7486:

  1. mne.io.pick.get_channel_types (source) takes no input, gets all known channel types as dict keys (values are FIFF constants for kind and coil_type).
  2. mne.io.pick._get_channel_types (source) takes an Info, picks, boolean unique, and boolean whether to restrict to data channels only. Returns a list or set depending on unique.
  3. <Raw/Epochs/Evoked>.get_channel_types (source) method (via the ContainsMixin). Returns list of channel types.

Number 1 is called once in a test, and nowhere else in the codebase. I'd like to propose renaming it from get_channel_types to get_all_channel_types, show_all_channel_types, or get_channel_types_dict. I prefer the first option. This is a public function so it would be an API change w/ deprecation cycle. If acceptable, I could roll it into #7486 or do it in a separate PR.

@larsoner larsoner added this to the 0.20 milestone Mar 21, 2020
@larsoner
Copy link
Member

Number 1 is called once in a test, and nowhere else in the codebase. I'd like to propose renaming it from get_channel_types to get_all_channel_types, show_all_channel_types, or get_channel_types_dict. I prefer the first option.

That was for @cbrnr in #4842 so perhaps he should comment. But I agree that inst.get_channel_types is probably more widely used and that the overlap is problematic, so +1 for renaming mne.io.pick.get_channel_types to get_all_channel_type_constants or something similar (a variant of names you suggested that makes it clearer it's giving a mapping between ch types and their constants, in this case via a dict)

@agramfort
Copy link
Member

agramfort commented Mar 21, 2020 via email

@cbrnr
Copy link
Contributor

cbrnr commented Mar 21, 2020

I'm fine with renaming the function, but I'd prefer a slightly shorter name if possible. Also, I haven't checked the return values of mne.io.pick.get_channel_types and mne.io.pick._get_channel_types, but can/should we de-duplicate code by calling the private function inside the public one?

@agramfort
Copy link
Member

agramfort commented Mar 21, 2020 via email

@drammock
Copy link
Member Author

Also, I haven't checked the return values of mne.io.pick.get_channel_types and mne.io.pick._get_channel_types, but can/should we de-duplicate code by calling the private function inside the public one?

Not possible, they do completely different things, which is why I proposed to rename one of them.

What I have done is deduplicate the instance methods and the private function, which were doing (roughly) the same thing.

drammock added a commit to drammock/mne-python that referenced this issue Mar 21, 2020
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

No branches or pull requests

4 participants