MAINT: Refactor _get_raw_paths #749

larsoner · 2023-06-28T16:42:17Z

Before merging …

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

Should be a no-op. Will push commits periodically to check that everything still works and ping when ready for review.

larsoner

Okay I think it's finally ready! It's hard to tell from the diff, but there is a lot of code deduplication and flattening of conditionals etc.

larsoner · 2023-06-29T18:23:38Z

mne_bids_pipeline/_config_utils.py

@@ -118,16 +118,6 @@ def get_sessions(config: SimpleNamespace) -> Union[List[None], List[str]]:
        return sessions


-@functools.lru_cache(maxsize=None)


The logic here was backward I think. The "public" get_runs_all_subjects should just cache under the hood so anytime we use it we benefit.

larsoner

@hoechenberger did you want to look or are you okay with trusting the green on this one? I've added some comments to help orient you. But really that plus looking at the files themselves (either in the browser with "View file" or locally with a checkout) is much easier than looking at the diff because you can't easily tell the dedents etc.

larsoner · 2023-06-30T14:07:13Z

mne_bids_pipeline/steps/preprocessing/_03_maxfilter.py

-        if filter_chpi:
-            logger.info(**gen_log_kwargs(message="Filtering cHPI", run=task))
-            # allow_line_only=True is really mostly for the "noise" run
-            mne.chpi.filter_chpi(raw_noise_sss, allow_line_only=True)


A bunch of DRY here

larsoner · 2023-06-30T14:07:22Z

mne_bids_pipeline/steps/preprocessing/_04_frequency_filter.py

-            raw_noise.compute_psd(fmax=fmax).plot()
-
-    assert len(in_files) == 0, in_files.keys()
-


And a bunch more DRY here

larsoner · 2023-06-30T14:08:05Z

mne_bids_pipeline/steps/preprocessing/_04_frequency_filter.py

            )
            for subject in get_subjects(config)
            for session in get_sessions(config)
-            for run in get_runs(config=config, subject=subject)
+            for run, task in get_runs_tasks(


The key change is to iterate over run, task pairs. So the experimental runs typically end up with run, cfg.task and then the noise and rest (if present) end up as None, "noise" and None, "rest" respectively

larsoner · 2023-06-30T14:09:17Z

mne_bids_pipeline/steps/preprocessing/_01_data_quality.py

-                run=run,
-                out_files=out_files,
-            )
+    key = f"raw_task-{task}_run-{run}"


And keys for the "data file being processed" are now standardized this way, as raw_task-{task}_run-{run}. The one exception is the reference run gets stored as raw_ref_run still, because it's in addition to / a companion of the data file being processed, and which run/task it's from is not relevant

larsoner · 2023-06-30T14:09:41Z

mne_bids_pipeline/steps/preprocessing/_01_data_quality.py

+    key = f"raw_task-{task}_run-{run}"
+    bids_path_in = in_files.pop(key)
+    if _do_mf_autobad(cfg=cfg):
+        if key == "raw_task-noise_run-None":


... so we can check for noise-run-ness using the standard key

larsoner · 2023-06-30T14:12:29Z

mne_bids_pipeline/_import_data.py

-                        )
-                    )
-                    raw_fname = raw_fname["fname"]
-                    if raw_fname is not None:


Got rid of 5 (!) levels of indentation by changing/refactoring the logic here. Hopefully it's (much) easier to follow when editing the files now...

larsoner · 2023-06-30T18:46:03Z

mne_bids_pipeline/_config.py

@@ -416,6 +416,14 @@
    ```
 """

+read_raw_bids_verbose: Optional[Literal["error"]] = None


Added this one because I was getting a bunch of warnings when processing ds000117:

/home/larsoner/python/mne-bids-pipeline/mne_bids_pipeline/_import_data.py:226: RuntimeWarning: The events file, /mnt/bakraid/data/mne_data/ds000117-full/sub-01/ses-meg/meg/sub-01_ses-meg_task-facerecognition_run-04_events.tsv, contains a "stim_type" column. This column should be renamed to "trial_type" for BIDS compatibility. raw = read_raw_bids( /home/larsoner/python/mne-bids-pipeline/mne_bids_pipeline/_import_data.py:226: RuntimeWarning: Did not find any channels.tsv associated with sub-01_ses-meg_task-facerecognition_run-04. The search_str was "/mnt/bakraid/data/mne_data/ds000117-full/sub-01/**/meg/sub-01_ses-meg*channels.tsv" raw = read_raw_bids( /home/larsoner/python/mne-bids-pipeline/mne_bids_pipeline/_import_data.py:226: RuntimeWarning: Did not find any meg.json associated with sub-01_ses-meg_task-facerecognition_run-04. The search_str was "/mnt/bakraid/data/mne_data/ds000117-full/sub-01/**/meg/sub-01_ses-meg*meg.json"

larsoner · 2023-06-30T19:19:54Z

mne_bids_pipeline/_run.py

+                        # generally be stuff from this file and joblib
+                        tb = tb[-fi:]
+                        break
+                tb = "".join(traceback.format_list(tb))


And this one gives much nicer tracebacks by omitting the _run.py and joblib stuff, so we get this instead of a 5- or 6-level traceback (here where I've added a raise RuntimeError to _02_find_empty_room):

[15:13:59] │ ❌ init/_02_find_empty_room sub-01 ses-meg run-02 A critical error occurred. The error message was: Aborting pipeline run. The full traceback is: File "/home/larsoner/python/mne-bids-pipeline/mne_bids_pipeline/steps/init/_02_find_empty_room.py", line 70, in find_empty_room raise RuntimeError

larsoner · 2023-06-30T19:21:00Z

mne_bids_pipeline/_run.py

    for frame in stack:
        fname = pathlib.Path(frame.filename)
        if "steps" in fname.parts:
            return fname
+        else:  # pragma: no cover


... and this can happen when you are running in parallel (e.g. with n_jobs=4). The __mne_bids_pipeline_step__ var will be used.

larsoner · 2023-06-30T19:21:23Z

mne_bids_pipeline/_run.py

@@ -29,7 +29,8 @@ def failsafe_run(
 ) -> Callable:
    def failsafe_run_decorator(func):
        @functools.wraps(func)  # Preserve "identity" of original function
-        def wrapper(*args, **kwargs):
+        def __mne_bids_pipeline_failsafe_wrapper__(*args, **kwargs):
+            __mne_bids_pipeline_step__ = pathlib.Path(inspect.getfile(func))  # noqa


... and we set the __mne_bids_pipeline_step__ here

hoechenberger · 2023-07-01T08:24:50Z

mne_bids_pipeline/_config.py

@@ -416,6 +416,14 @@
    ```
 """

+read_raw_bids_verbose: Optional[Literal["error"]] = None


Could you mention this new setting in the changelog, please?

hoechenberger · 2023-07-03T16:14:29Z

@larsoner LMK when you think it's ready for merge / review :)

larsoner · 2023-07-03T17:18:53Z

@hoechenberger yes it should be ready!

larsoner · 2023-07-03T17:19:42Z

(I just ran all of ds000117 with mf_mc = True on this branch -- it's what prompted some of the actual changes like read_raw_bids_verbose and the __mne_bids_pipeline_step__!)

agramfort

@hoechenberger I let you merge if happy

thx @larsoner

hoechenberger

Fantastic

larsoner added 6 commits June 28, 2023 12:42

MAINT: Refactor _get_raw_paths

2ed2892

WIP

07da439

MAINT: Flatten logic

c5c5b75

FIX: Missed

831ee94

FIX: Logic

5f59817

FIX: Fine

f080aa5

larsoner mentioned this pull request Jun 27, 2023

ENH: Add movement compensation and related functions #574

Open

12 tasks

larsoner added 12 commits June 28, 2023 15:07

FIX: Logic

ad07f3f

FIX: Working?

70d46f7

FIX: One more

955f24d

TST: Still broken [ci skip]

5509b76

WIP

8bdc8a4

WIP: Closer

10e5eee

FIX: Maybe?

a6f9066

FIX: Better

6f0a458

FIX: WIP

c30f4c4

FIX: Working

5407c62

FIX: Local

a3cc2d6

ENH: Ruler

6dccd5c

larsoner commented Jun 29, 2023

View reviewed changes

larsoner marked this pull request as ready for review June 29, 2023 19:09

larsoner added 3 commits June 29, 2023 15:10

FIX: Emoji

3092b78

FIX: Memory

93fb70b

FIX: Class

ac8cffc

larsoner commented Jun 30, 2023

View reviewed changes

FIX: Cleaner

871e863

larsoner commented Jun 30, 2023

View reviewed changes

larsoner added 3 commits June 30, 2023 15:01

FIX: Parallel fix

60ccddd

FIX: Much shorter traceback

85ea8c8

FIX: Verbose

f907ef5

larsoner commented Jun 30, 2023

View reviewed changes

FIX: More verbose

185b0f8

hoechenberger reviewed Jul 1, 2023

View reviewed changes

larsoner added 2 commits July 1, 2023 12:41

DOC: read_raw_bids_verbose

fa4ea95

Merge branch 'main' into refactor

c0e6fd1

agramfort approved these changes Jul 3, 2023

View reviewed changes

hoechenberger approved these changes Jul 3, 2023

View reviewed changes

hoechenberger merged commit fffe29f into mne-tools:main Jul 3, 2023

larsoner deleted the refactor branch July 3, 2023 20:14

hoechenberger mentioned this pull request Jul 3, 2023

BUG: Website too large to deploy #752

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MAINT: Refactor _get_raw_paths #749

MAINT: Refactor _get_raw_paths #749

larsoner commented Jun 28, 2023 •

edited

Loading

larsoner left a comment

larsoner Jun 29, 2023

larsoner left a comment

larsoner Jun 30, 2023

larsoner Jun 30, 2023

larsoner Jun 30, 2023

larsoner Jun 30, 2023

larsoner Jun 30, 2023

larsoner Jun 30, 2023

larsoner Jun 30, 2023 •

edited

Loading

larsoner Jun 30, 2023

larsoner Jun 30, 2023

larsoner Jun 30, 2023

hoechenberger Jul 1, 2023

larsoner Jul 1, 2023

hoechenberger commented Jul 3, 2023

larsoner commented Jul 3, 2023

larsoner commented Jul 3, 2023

agramfort left a comment

hoechenberger left a comment

		@@ -118,16 +118,6 @@ def get_sessions(config: SimpleNamespace) -> Union[List[None], List[str]]:
		return sessions


		@functools.lru_cache(maxsize=None)

		raw_noise.compute_psd(fmax=fmax).plot()

		assert len(in_files) == 0, in_files.keys()

MAINT: Refactor _get_raw_paths #749

MAINT: Refactor _get_raw_paths #749

Conversation

larsoner commented Jun 28, 2023 • edited Loading

Before merging …

larsoner left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

larsoner left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

larsoner Jun 30, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hoechenberger commented Jul 3, 2023

larsoner commented Jul 3, 2023

larsoner commented Jul 3, 2023

agramfort left a comment

Choose a reason for hiding this comment

hoechenberger left a comment

Choose a reason for hiding this comment

larsoner commented Jun 28, 2023 •

edited

Loading

larsoner Jun 30, 2023 •

edited

Loading