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

ENH : add units parameter to read_raw_edf in case units is missing from the file #11099

Merged
merged 5 commits into from
Aug 27, 2022

Conversation

agramfort
Copy link
Member

related to mne-tools/mne-bids#1045 and mne-tools/mne-bids#1044

now with the data from ds003775 you can just do:

raw = mne.io.read_raw_edf(bp, units='uV')

and in mne-bids

raw = read_raw_bids(bp, extra_params={'units': 'uV'})

what do you think @sappelhoff @hoechenberger ?

@hoechenberger
Copy link
Member

hoechenberger commented Aug 26, 2022

@agramfort Looks good, but I'd call the parameter unit (singular), since it's just one unit (i.e., the same one) for all channels in the most common scenario (user passes a string). WDYT?

An alternative approach could also be to introduce a parameter scaling instead? And accept an an int or float by which we'll multiply the input data? I feel this would fit better into the existing MNE-Python API, but it's slightly less convenient for users (?)

What bugs me a little bit about calling the parameter unit(s) is that we don't actually perform a unit conversion, but simply adjust the scaling factor. Hence the idea of having scaling instead.

@sappelhoff
Copy link
Member

LGTM!

I like "units" (plural), because one can also pass a dict of units (one for each channel).
Regarding "scaling(s)" --> true, we are not doing unit conversions ... but won't get_scalings warn/error when a user passes a unit that does not correspond to the unit the channel type is in?

Still, I'd be fine with bot units and scalings.

@hoechenberger
Copy link
Member

I like "units" (plural), because one can also pass a dict of units (one for each channel).

But I believe we usually follow the convention that variable names for dictionaries are singular, while names for lists are plural, no?

I'm not vetoing "units", just thinking out loud.

@agramfort
Copy link
Member Author

@hoechenberger do a git grep in the repo with units. You will see that we call units many parameters that are dict of units for the different channels. I think this naming is consistent with mne

@hoechenberger hoechenberger merged commit 6cebb13 into mne-tools:main Aug 27, 2022
@hoechenberger
Copy link
Member

Thanks @agramfort!

@hoechenberger
Copy link
Member

Oh, I merged without requesting a test.

@agramfort Care to add a small test case in a followup PR?

larsoner added a commit to larsoner/mne-python that referenced this pull request Aug 29, 2022
* upstream/main:
  Revert "Add error message when conversion of EEG locs to [circle deploy] (mne-tools#11104)
  MRG: Fixes for mne-tools#11090 (mne-tools#11108)
  add test for edf units param (mne-tools#11105)
  BUG: Improve logic for bti (mne-tools#11102)
  add spectrum class (mne-tools#10184)
  ENH : add units parameter to read_raw_edf in case units is missing from the file (mne-tools#11099)
  ENH: Add temperature and galvanic (mne-tools#11090)
larsoner added a commit to larsoner/mne-python that referenced this pull request Aug 29, 2022
* upstream/main: (366 commits)
  BUG: Spectrum deprecation cleanup [circle deploy] (mne-tools#11115)
  Add API entry list and map (mne-tools#10999)
  Add legacy decorator (mne-tools#11097)
  [ENH, MRG] Add time-frequency epoch source estimation (mne-tools#11095)
  Revert "Add error message when conversion of EEG locs to [circle deploy] (mne-tools#11104)
  MRG: Fixes for mne-tools#11090 (mne-tools#11108)
  add test for edf units param (mne-tools#11105)
  BUG: Improve logic for bti (mne-tools#11102)
  add spectrum class (mne-tools#10184)
  ENH : add units parameter to read_raw_edf in case units is missing from the file (mne-tools#11099)
  ENH: Add temperature and galvanic (mne-tools#11090)
  Add error message when conversion of EEG locs to head space fails (mne-tools#11080)
  DOC: removed unnecessary line in PSF example (mne-tools#11085)
  FIX: Update helmet during ICP (mne-tools#11084)
  Fix various typos (mne-tools#11086)
  DOC: Rel
  BUG: don't assume that channel info contains particular keys (mne-tools#11074)
  [BUG] Fix plot_topomap with sphere="eeglab" (mne-tools#11081)
  Add `vmin` and `vmax` specification to `mne.Evoked.animate_topomap` (mne-tools#11073)
  Add _on_missing functionality to UpdateChannelsMixin (mne-tools#11077)
  ...
larsoner added a commit to larsoner/mne-python that referenced this pull request Aug 30, 2022
* upstream/main: (25 commits)
  DOC: Exclude some implicit refs in doc build (mne-tools#10433)
  MAINT: Better issue forms (mne-tools#11101)
  [FIX] Typo in example (mne-tools#11118)
  [MRG] Improve interpolation of bridged electrodes (mne-tools#11094)
  BUG: Spectrum deprecation cleanup [circle deploy] (mne-tools#11115)
  Add API entry list and map (mne-tools#10999)
  Add legacy decorator (mne-tools#11097)
  [ENH, MRG] Add time-frequency epoch source estimation (mne-tools#11095)
  Revert "Add error message when conversion of EEG locs to [circle deploy] (mne-tools#11104)
  MRG: Fixes for mne-tools#11090 (mne-tools#11108)
  add test for edf units param (mne-tools#11105)
  BUG: Improve logic for bti (mne-tools#11102)
  add spectrum class (mne-tools#10184)
  ENH : add units parameter to read_raw_edf in case units is missing from the file (mne-tools#11099)
  ENH: Add temperature and galvanic (mne-tools#11090)
  Add error message when conversion of EEG locs to head space fails (mne-tools#11080)
  DOC: removed unnecessary line in PSF example (mne-tools#11085)
  FIX: Update helmet during ICP (mne-tools#11084)
  Fix various typos (mne-tools#11086)
  DOC: Rel
  ...
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