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

different runs for different subjects #353

Merged
merged 27 commits into from
Jun 2, 2021

Conversation

crsegerie
Copy link
Contributor

@crsegerie crsegerie commented May 28, 2021

Closes #352.

During the process, I've moved up the get_subjects() function without changing it.

Before merging …

  • Changelog has been updated (docs/source/changes.md)

Copy link
Member

@agramfort agramfort left a comment

Choose a reason for hiding this comment

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

is it already functional on your end @crsegerie ?

config.py Show resolved Hide resolved
Copy link
Member

@hoechenberger hoechenberger left a comment

Choose a reason for hiding this comment

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

Left some small suggestions

Is this already working as intended for you locally? :)

config.py Outdated Show resolved Hide resolved
config.py Outdated Show resolved Hide resolved
config.py Outdated Show resolved Hide resolved
docs/source/changes.md Outdated Show resolved Hide resolved
config.py Outdated Show resolved Hide resolved
config.py Show resolved Hide resolved
scripts/preprocessing/02-frequency_filter.py Outdated Show resolved Hide resolved
crsegerie and others added 3 commits May 29, 2021 10:50
change return type

Co-authored-by: Richard Höchenberger <richard.hoechenberger@gmail.com>
change docstring if None return

Co-authored-by: Richard Höchenberger <richard.hoechenberger@gmail.com>
docstring update

Co-authored-by: Richard Höchenberger <richard.hoechenberger@gmail.com>
@crsegerie
Copy link
Contributor Author

crsegerie commented May 29, 2021

is it already functional on your end @crsegerie ?

Not quite.

There is still an error on my own :

02:08:52 [Step-03][sub-403, run-07] Loading filtered raw data from /home/charb/Desktop/parietal/time_in_wm/derivatives/mne-bids-pipeline/sub-403/meg/sub-403_task-wm_run-07_proc-filt_raw.fif
02:08:53 [Step-03][sub-403, run-08] Loading filtered raw data from /home/charb/Desktop/parietal/time_in_wm/derivatives/mne-bids-pipeline/sub-403/meg/sub-403_task-wm_run-08_proc-filt_raw.fif
Killed

The killed surprises me, because I do not see it in CI

And at the beginning of each step, the warning appears:

01:58:28 Extracted all the runs. Beware, not all subjects share the same list of run-names.

I should change it to only warn for the first step

Co-authored-by: Alexandre Gramfort <alexandre.gramfort@m4x.org>
@hoechenberger
Copy link
Member

hoechenberger commented May 29, 2021

Killed

This most likely indicates that you're running out of memory … We'd need to see how to optimize things here. This problem seems unrelated to this PR though

@hoechenberger
Copy link
Member

hoechenberger commented May 29, 2021

@agramfort Do you think instead of concatenating all filtered runs before creating epochs, we could create epochs for each run individually and then concat those? This would avoid the out-of-memory problem @crsegerie sees locally. 8+ runs is heavy stuff.

@hoechenberger
Copy link
Member

@agramfort Do you think instead of concatenating all filtered runs before creating epochs, we could create epochs for each run individually and then concat those?

Ah, right, there is mne.concatenate_epochs()

I can take a stab at this sometime later today (separate PR)

config.py Show resolved Hide resolved
config.py Show resolved Hide resolved
config.py Outdated Show resolved Hide resolved
Co-authored-by: Alexandre Gramfort <alexandre.gramfort@m4x.org>
@crsegerie crsegerie marked this pull request as ready for review May 31, 2021 07:42
@crsegerie
Copy link
Contributor Author

There is a conflict I'm not sure how to deal with :

image

I do not understand the MKDOC part ?

@hoechenberger
Copy link
Member

I do not understand the MKDOC part ?

I added this MKDOCS check to make it work with our documentation build, which uses MkDocs. It's a bit hacky but seems to work: When building the docs, we set this environment variable so the config "knows": "oh ok, so this is a doc build, not an actual run of the pipeline."

config.py Outdated Show resolved Hide resolved
crsegerie and others added 4 commits May 31, 2021 20:38
delete parentheses

Co-authored-by: Richard Höchenberger <richard.hoechenberger@gmail.com>
config.py Outdated Show resolved Hide resolved
Copy link
Member

@hoechenberger hoechenberger left a comment

Choose a reason for hiding this comment

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

Just some nitpicks re docstrings, otherwise LGTM

config.py Outdated


def get_intersect_run() -> list:
'''Returns the intersection of all the runs of all subjects.'''
Copy link
Member

Choose a reason for hiding this comment

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

Can you use double-quotes """xxxx""" around the docstring please?

config.py Outdated Show resolved Hide resolved
config.py Outdated Show resolved Hide resolved
@hoechenberger hoechenberger merged commit d4061dc into mne-tools:main Jun 2, 2021
@hoechenberger
Copy link
Member

Great job, @crsegerie!!!

@crsegerie crsegerie deleted the different-runs-by-subjects branch June 2, 2021 08:12
@agramfort
Copy link
Member

agramfort commented Jun 2, 2021 via email

@agramfort
Copy link
Member

🎉

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.

runs = "all" assumes that all participants have the same runs
3 participants