Skip to content

Conversation

@drammock
Copy link
Member

expands the entries in get_channel_type_constants() to include coil types and units, so they can be used in create_info.

@drammock drammock changed the title MAINT: deduplicate definintion of FIFF constants MAINT: deduplicate definition of FIFF constants Nov 17, 2020
Copy link
Member

@agramfort agramfort left a comment

Choose a reason for hiding this comment

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

thx @drammock

@larsoner
Copy link
Member

Failures look real

@drammock
Copy link
Member Author

Failures look real

yep. ran out of steam last night. working on them now.

@drammock drammock force-pushed the dedup-FIFF-constants branch from 4753223 to cc2379b Compare November 18, 2020 17:08

def get_channel_type_constants():
"""Return all known channel types.
def get_channel_type_constants(include_defaults=False):
Copy link
Member Author

Choose a reason for hiding this comment

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

@larsoner could you have a look at how I've altered this function? As I understand it: sometimes we want the returned dict to have minimal entries (values that should always be present in info['chs'] for a given channel type, regardless of the acquisition system used). Other times, we want additional values because we use the returned dict to pre-populate an info dict for synthetic data (e.g., in create_info). Does that understanding accurately reflect all of the relevant use cases for this kind of dictionary? If so, does the approach I've taken here seem optimal?

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.

Otherwise LGTM. Not 100% sure on the include_defaults stuff but what you did looks reasonable



FILE = inspect.getfile(inspect.currentframe())
KIND_DICT = 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.

I'd rather nest this in test_edf_stim_ch_pick_up -- if we ever inadvertently break get_channel_type_constants, it will break tests during the collection rather than the running phase, which is not so nice.

@drammock drammock changed the title MAINT: deduplicate definition of FIFF constants MRG, MAINT: deduplicate definition of FIFF constants Nov 19, 2020
@drammock
Copy link
Member Author

CIs all green, ready for review/merge.

@larsoner larsoner merged commit 02db0b8 into mne-tools:master Nov 19, 2020
@larsoner
Copy link
Member

Thanks @drammock

larsoner added a commit to kylemath/mne-python that referenced this pull request Nov 19, 2020
* upstream/master:
  MRG, MAINT: Try conda-forge (mne-tools#8046)
  MRG, MAINT: deduplicate definition of FIFF constants (mne-tools#8537)
  ENH: Add realign_raw (mne-tools#8540)
  CI: Use 20.04 (mne-tools#8541)
  fix example (mne-tools#8539)
  MRG, VIZ, FIX: plot_sensors title and interactivity (mne-tools#8536)
  MRG, MAINT: Test 3.9 (mne-tools#8533)
@drammock drammock deleted the dedup-FIFF-constants branch November 19, 2020 17:24
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.

3 participants