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

Fix: nk.data import of eeg sample data #1072

Open
wants to merge 1 commit into
base: dev
Choose a base branch
from

Conversation

DerAndereJohannes
Copy link
Collaborator

@DerAndereJohannes DerAndereJohannes commented Feb 11, 2025

TL;DR

Description

This PR fixes the documentation issue as one of the files uses the sample eeg data. The problem stems from the most recent version of mne which has a function which removes the filenames (and checks for their existence on disk!!) when trying to crop the data. The motivation here seems to be that it can remove the filenames if the crop range is out of scope of a file.

This caused issues, since the current version of the eeg sample data is imported via a pickle rather than eeg data itself. This included metadata that was relevant for the uploader, but of course the file name does not exist in this case.

Due to this, it was not possible to use the sample data and therefore the documentation pipeline could not work.

Proposed Changes

The current proposal is to check for the version number and if it is greater than or equal to 1.9, then it removes the filenames from the eeg data. The change makes sense as there is in fact no filename connected to the in-memory file that came from the pickle, however, I am also not too sure if it looks too "hacky".

Checklist

Here are some things to check before creating the PR. If you encounter any issues, do let us know :)

  • I have read the CONTRIBUTING file.
  • My PR is targeted at the dev branch (and not towards the master branch).
  • I ran the CODE CHECKS on the files I added or modified and fixed the errors.
  • I have added the newly added features to News.rst (if applicable)

- Mne 1.9 changes how cropping works to include file checks
- Sample data is stored in pickle so does not come from disk
- Change removes previous existing filenames from eeg sample data
@DerAndereJohannes
Copy link
Collaborator Author

DerAndereJohannes commented Feb 11, 2025

The documentation workflow works on my machine. I realise now that the documentation workflow does its check based on the dev branch so the update is not reflected here (perhaps this should be changed too?).

I will look more into why the other checks seem to be failing at a later time.

DominiqueMakowski added a commit that referenced this pull request Feb 12, 2025
@DominiqueMakowski
Copy link
Member

I've updated the dataset on dev

import pickle

import mne

raw = mne.io.read_raw_fif(
    mne.datasets.sample.data_path() / "MEG/sample/sample_audvis_raw.fif",
    preload=True,
    verbose=False,
)
raw = raw.pick(["eeg", "eog", "stim"], verbose=False)
raw = raw.crop(0, 60)
raw = raw.resample(200)

# raw.ch_names

# raw.info["sfreq"]

# Store data (serialize)
with open("eeg_1min_200hz.pickle", "wb") as handle:
    pickle.dump(raw, handle, protocol=pickle.HIGHEST_PROTOCOL)

and thought that would do the trick (rather than fixing in in the data function) but apparently not -_-

@DominiqueMakowski
Copy link
Member

And I can't reproduce the error in the eeg_microstates notebook locally

@DerAndereJohannes
Copy link
Collaborator Author

The issue I think is that the eeg_microstates notebook still uses the .crop(0, 5) function on the data. Every crop on the data will check the existence of the file so the issue remains (check the filenames by doing print(raw.filenames) in your code).

It works on your machine, because the filenames are in fact correct to your setup. It's because of this I know that your username on your PC is "domma" ;-)

@DominiqueMakowski
Copy link
Member

oooooh right I see, good catch! I'll try to replace .crop() with our mne_crop() function which recreates a new object from scratch, maybe that'll help

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants