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

Changed casting rule in np.clip to allow reading of raw GDF files #12168

Merged
merged 3 commits into from
Nov 3, 2023
Merged

Changed casting rule in np.clip to allow reading of raw GDF files #12168

merged 3 commits into from
Nov 3, 2023

Conversation

rasgaard
Copy link
Contributor

@rasgaard rasgaard commented Nov 3, 2023

When reading in raw GDF files I got the following error

UFuncTypeError: Cannot cast ufunc 'clip' output from dtype('float64') to dtype('uint32') with casting rule 'same_kind'

Referring to the usage of np.clip here:

mne-python/mne/io/edf/edf.py

Line 1446 in 87d6be9

np.clip(dur, 1, np.inf, out=dur)

This error can reproduced noting that np.inf has dtype of float64.

import numpy as np
dur = np.ones(10, dtype=np.uint32)
np.clip(dur, 1, np.inf, out=dur)
>>> UFuncTypeError: Cannot cast ufunc 'clip' output from dtype('float64') to dtype('uint32') with casting rule 'same_kind'

Changing aforementioned line 1446 to np.clip(dur, 1, np.inf, out=dur, casting="unsafe") seemingly fixes this issue.

Additionally, there is a typo when using the read_raw_gdf-function on files that does not have the gdf-file extenion

mne-python/mne/io/edf/edf.py

Line 1881 in 87d6be9

raise NotImplementedError(f"Only BDF files are supported, got {ext}.")

@rasgaard
Changed casting rule in np.clip to allow reading of raw GDF files
8aecfac 
+ a type in error messag from the `read_raw_gdf` functions extension checking
@welcome Welcome
Copy link

welcome bot commented Nov 3, 2023

Hello! 👋 Thanks for opening your first pull request here! ❤️ We will try to get back to you soon. 🚴

larsoner
larsoner reviewed Nov 3, 2023
View reviewed changes
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.

Can you also add an entry to doc/changes/devel.rst using the :newcontrib: role and also a name-url pair to doc/changes/names.inc?

mne/io/edf/edf.py Outdated
@@ -1443,7 +1443,7 @@ def _read_gdf_header(fname, exclude, include=None):
else:
chn = np.zeros(n_events, dtype=np.uint32)
dur = np.ones(n_events, dtype=np.uint32)
np.clip(dur, 1, np.inf, out=dur)
np.clip(dur, 1, np.inf, out=dur, casting="unsafe")
Copy link
Member

Choose a reason for hiding this comment

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

I'm not really sure what purpose the upper bound is meant to serve, but to me this is a cleaner / more explicit fix

                np.clip(dur, 1, np.iinfo(dur.dtype).max, out=dur, casting="unsafe")

But really there is no point in checking the max here since it will always be satisfied. So really we should probably do:

Suggested change
np.clip(dur, 1, np.inf, out=dur, casting="unsafe")
np.maximum(dur, 1, out=dur)

Copy link
Member

Choose a reason for hiding this comment

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

... and actually this np.maximum / np.clip step can live in the first branch of the conditional above if you want. There is no point in running this unless etmode == 3 because in the else we construct it to satisfy the maximum check with dur = np.ones(n_events, dtype=np.uint32).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I noticed that

mne-python/mne/io/edf/edf.py

Line 1194 in 87d6be9

np.maximum(dur, 1, out=dur)
uses np.maximum in the exact way you deduced your way through. I changed my commit to mimic that instead :)

larsoner
larsoner approved these changes Nov 3, 2023
View reviewed changes
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.

Marking for merge-when-green, thanks in advance @rasgaard !

@larsoner larsoner enabled auto-merge (squash) November 3, 2023 17:21
@larsoner larsoner merged commit f02e557 into mne-tools:main Nov 3, 2023
28 checks passed
@welcome Welcome
Copy link

welcome bot commented Nov 3, 2023

🎉 Congrats on merging your first pull request! 🥳 Looking forward to seeing more from you in the future! 💪

larsoner added a commit to JD-Zhu/mne-python that referenced this pull request Nov 3, 2023
@larsoner
Merge remote-tracking branch 'upstream/main' into jdz
cf08258 
* upstream/main: (26 commits)
  FIX: Fix bug with coreg scalars (mne-tools#12164)
  Changed casting rule in np.clip to allow reading of raw GDF files (mne-tools#12168)
  [DOC] Add documentation for setting montage order (mne-tools#12160)
  Fix inferring fiducials from EEGLAB (mne-tools#12165)
  Try to fix ICA Report (mne-tools#12167)
  BUG: Fix bug with Report.add_ica component number (mne-tools#12156)
  MAINT: Add rstcheck to CIs and pre-commit (mne-tools#12163)
  DOC: fix sphinx style typos (mne-tools#12161)
  MAINT: Fix linkcheck (mne-tools#12162)
  ENH: Add multiple label support to source_band_induced_power, source_induced_power (mne-tools#12026)
  Allow automated metadata generation to be bounded by "row events" instead of explicit time windows (mne-tools#12118)
  ENH: Collapse only in doc gen (mne-tools#12145)
  [pre-commit.ci] pre-commit autoupdate (mne-tools#12155)
  BUG: Fix bug with interior points not showing (mne-tools#12148)
  ENH: Warn about versions in sys_info (mne-tools#12146)
  Fix in conftest.py (mne-tools#12150)
  ENH: set color for bad channel with spatial_colors=True (mne-tools#12142)
  DOC: Better documentation of realign_raw (mne-tools#12135)
  Add mne-icalabel wildcard (mne-tools#12143)
  Remove LGTM.com configuration file (mne-tools#12139)
  ...
larsoner added a commit to larsoner/mne-python that referenced this pull request Nov 6, 2023
@larsoner
Merge remote-tracking branch 'upstream/main' into kernel
ec50c88 
* upstream/main:
  MAINT: Add branch coverage (mne-tools#12174)
  OpenSSF (mne-tools#12175)
  fix docstring in 60_sleep.py (mne-tools#12171)
  FIX: skip empty lines in read_raw_eyelink (mne-tools#12172)
  FIX: Fix bug with coreg scalars (mne-tools#12164)
  Changed casting rule in np.clip to allow reading of raw GDF files (mne-tools#12168)
larsoner added a commit to pablomainar/mne-python that referenced this pull request Nov 8, 2023
@larsoner
Merge remote-tracking branch 'upstream/main' into fix
31c299b 
* upstream/main:
  BUG: Fix bug with spectrum warning (mne-tools#12186)
  Add argument splash to disable splash-screen from Qt-browser (mne-tools#12185)
  BUG: Fix bug with logging and n_jobs>1 (mne-tools#12154)
  Use gray logo (works in light and dark modes) (mne-tools#12184)
  Tweak logo for dark mode (mne-tools#12176)
  ENH: Improve Covariance.__repr__ (mne-tools#12181)
  ENH: Enable sensor-specific OPM coregistration in mne coreg (mne-tools#11405)
  Tweak README.rst (mne-tools#12166)
  [pre-commit.ci] pre-commit autoupdate (mne-tools#12177)
  MAINT: Add branch coverage (mne-tools#12174)
  OpenSSF (mne-tools#12175)
  fix docstring in 60_sleep.py (mne-tools#12171)
  FIX: skip empty lines in read_raw_eyelink (mne-tools#12172)
  FIX: Fix bug with coreg scalars (mne-tools#12164)
  Changed casting rule in np.clip to allow reading of raw GDF files (mne-tools#12168)
  [DOC] Add documentation for setting montage order (mne-tools#12160)
  Fix inferring fiducials from EEGLAB (mne-tools#12165)
snwnde pushed a commit to snwnde/mne-python that referenced this pull request Mar 20, 2024
@rasgaard @snwnde
Changed casting rule in np.clip to allow reading of raw GDF files (mn…
19ad897 
…e-tools#12168)

Co-authored-by: roraa <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@larsoner larsoner larsoner 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.
2 participants
@rasgaard @larsoner