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.

Sign up for GitHub

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

Jump to bottom

MRG: Fixes for #11090 #11108

Merged
merged 2 commits into from
Aug 28, 2022
Merged

MRG: Fixes for #11090 #11108

merged 2 commits into from
Aug 28, 2022

Conversation

hoechenberger
Copy link
Member

@larsoner I discovered a bug (mapping temperature to Coulomb instead of Celsius) and an oversight in #11090 when trying to actually read GSR and temperature data.

@hoechenberger hoechenberger changed the title Fixes for #11090 MRG: Fixes for #11090 Aug 28, 2022
hoechenberger added a commit to hoechenberger/mne-bids that referenced this pull request Aug 28, 2022
@hoechenberger
Add support for GSR and temperature channels
bf348eb 
Also updates tiny_bids

Will fail until mne-tools/mne-python#11108 has been merged

Fixes mne-tools#1046
@hoechenberger hoechenberger mentioned this pull request Aug 28, 2022
Merged
7 tasks
hoechenberger
hoechenberger commented Aug 28, 2022
View reviewed changes
mne/channels/channels.py
@@ -202,7 +202,8 @@ def equalize_channels(instances, copy=True, verbose=None):
FIFF.FIFF_UNIT_T_M: 'T/m',
FIFF.FIFF_UNIT_MOL: 'M',
FIFF.FIFF_UNIT_NONE: 'NA',
FIFF.FIFF_UNIT_CEL: 'C'}
FIFF.FIFF_UNIT_CEL: 'C',
Copy link
Member Author

@hoechenberger hoechenberger Aug 28, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Are we bound to a string without spaces and no special characters? Can we change this to "degrees C" or (better) "°C"?

@agramfort
Copy link
Member

agramfort commented Aug 28, 2022 via email

@agramfort agramfort merged commit 9385b96 into mne-tools:main Aug 28, 2022
@agramfort
Copy link
Member

thx @hoechenberger

@hoechenberger
Copy link
Member Author

hoechenberger commented Aug 28, 2022 •
edited
Loading

let's stick to C to be consistent. It's the SI unit.

Well no, it's not, that's why I'm asking because it's bugging me so much. 🫠 The SI unit is degree Celsius, °C (plural form is degrees Celsius)

C is Coulomb. So using C for temperature is factually wrong

@hoechenberger hoechenberger deleted the fixes branch August 28, 2022 22:21
@agramfort
Copy link
Member

agramfort commented Aug 29, 2022 via email

@hoechenberger
Copy link
Member Author

hoechenberger commented Aug 29, 2022 •
edited
Loading

BIDS says to use "oC" see
https://bids-specification.readthedocs.io/en/stable/99-appendices/05-units.html

Message ID: @.***>

Omg, this has got to be the most unfortunate choice ... I thought one of the focus points of BIDS was on human-readable metadata ... I bet if I asked my lab colleagues, none of them would know what "oC" is supposed to mean :( but it's in the spec now and I don't have the time and energy to push for a change, so this is just what we'll have to live with for BIDS

But I'd really not want to introduce this ... atrocity... into MNE 😅
If we cannot have the degree sign (can we not?), we could call the unit "degC". WDYT?

@larsoner
Copy link
Member

We could try the unicode degree. I think we only really use this unit mapping for plotting, and for converting scale factors (e.g., fT to T). Both of those should work I think, maybe with some minor adjustments for the two characters in the units.

larsoner added a commit to larsoner/mne-python that referenced this pull request Aug 29, 2022
@larsoner
Merge remote-tracking branch 'upstream/main' into openmeeg
49c9474 
* 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)
@agramfort
Copy link
Member

agramfort commented Aug 29, 2022 via email

larsoner added a commit to larsoner/mne-python that referenced this pull request Aug 29, 2022
@larsoner
Merge remote-tracking branch 'upstream/main' into skip
99fa0c1 
* 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
@larsoner
Merge remote-tracking branch 'upstream/main' into volpc
4bac62a 
* 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
  ...
agramfort pushed a commit to mne-tools/mne-bids that referenced this pull request Aug 31, 2022
@hoechenberger @sappelhoff
MRG: Add support for GSR and temperature channels (#1059)
388616f 
* Add support for GSR and temperature channels

Also updates tiny_bids

Will fail until mne-tools/mne-python#11108 has been merged

Fixes #1046

* Indent

* Preserve info

* Require MNE devel to run tests

* Fix unit writing

* Write correct BIDS temperature unit (oC)

* Better code

* Add conditional

* No warning

* Skip test on MNE stable

* Decorated the wrong test

* Add channel statuses

* Update doc/whats_new.rst

Co-authored-by: Stefan Appelhoff <stefan.appelhoff@mailbox.org>

* Fix channel description

* Manually curate tiny_bids to keep the diff small

* Revert changes related to _orig_units handling

* Fix test

Co-authored-by: Stefan Appelhoff <stefan.appelhoff@mailbox.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@agramfort agramfort agramfort approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@hoechenberger @agramfort @larsoner