Skip to content

MRG: Fixes for #11090#11108

Merged
agramfort merged 2 commits intomne-tools:mainfrom
hoechenberger:fixes
Aug 28, 2022
Merged

MRG: Fixes for #11090#11108
agramfort merged 2 commits intomne-tools:mainfrom
hoechenberger:fixes

Conversation

@hoechenberger
Copy link
Copy Markdown
Member

@hoechenberger hoechenberger commented Aug 28, 2022

@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
FIFF.FIFF_UNIT_MOL: 'M',
FIFF.FIFF_UNIT_NONE: 'NA',
FIFF.FIFF_UNIT_CEL: 'C'}
FIFF.FIFF_UNIT_CEL: 'C',
Copy link
Copy Markdown
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
Copy Markdown
Member

agramfort commented Aug 28, 2022 via email

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

agramfort commented Aug 28, 2022

thx @hoechenberger

@hoechenberger
Copy link
Copy Markdown
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
Copy Markdown
Member

agramfort commented Aug 29, 2022 via email

@hoechenberger
Copy link
Copy Markdown
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
Copy Markdown
Member

larsoner commented Aug 29, 2022

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
Copy Markdown
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 <[email protected]>

* 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 <[email protected]>
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