Improvements to the windowers #152

hubertjb · 2020-08-08T17:00:53Z

This PR introduces some minor improvements to the windowers:

fixing windower functions when first_samp is not 0 (i.e., the Raw object has been cropped)
adding epochs_kwargs to windowing functions
modifying default value of stop_offset_samples to None for consistency
improving docstrings
adding random state argument to create_mne_dummy_raw
adding tests

- adding `epochs_kwargs` to windowing functions - modifying default value of stop_offset_samples to None for consistency - improving docstrings - adding random state argument to create_mne_dummy_raw - adding tests

agramfort · 2020-08-12T15:15:00Z

braindecode/datautil/windowers.py

@@ -154,7 +168,7 @@ def create_windows_from_events(
 def create_fixed_length_windows(
        concat_ds, start_offset_samples, stop_offset_samples,
        window_size_samples, window_stride_samples, drop_last_window,
-        mapping=None, preload=False, drop_bad_windows=True):
+        mapping=None, preload=False, drop_bad_windows=True, **epochs_kwargs):


i don't like kwargs. I would expose and document here the Epochs parameters you think can be useful.

Fixed in new commit.

agramfort · 2020-08-12T15:15:58Z

braindecode/datautil/xy.py

@@ -69,7 +69,7 @@ def create_from_X_y(
    windows_datasets = create_fixed_length_windows(
        base_datasets,
        start_offset_samples=0,
-        stop_offset_samples=0,
+        stop_offset_samples=None,


API change?

That's correct. Is there a recommended way to flag API changes? I added a warning here: https://github.com/braindecode/braindecode/pull/152/files#diff-c031004527cfc6287fbb1741b7f3e1ddR231

I think with warning makes sense for now? unless somebody suggests sth more sophisticated :)

agramfort · 2020-08-12T15:16:28Z

braindecode/util.py

@@ -250,7 +252,10 @@ def create_mne_dummy_raw(n_channels, n_times, sfreq, include_anns=True,
    save_fname : dict | None
        Dictionary containing the name the raw data was saved to.
    """
-    data = np.random.rand(n_channels, n_times)
+    if random_state is None or isinstance(random_state, int):
+        random_state = np.random.RandomState(random_state)


use sklearn.utils.check_random_state

test/unit_tests/datautil/test_windowers.py

- using sklearn.utils.check_random_state - updating corresponding test

agramfort · 2020-08-12T20:09:49Z

What about we rename braindecode.datautil.preprocess to braindecode.datautil.preprocessing? It's a bit longer, but at least we avoid this ambiguity. @robintibor <https://github.com/robintibor> @gemeinl <https://github.com/gemeinl>

3 levels of modules seem too much for a small package as it is now. I would expose it in braindecode.datautil or consider a new submodule braindecode.preprocessing

robintibor · 2020-08-14T15:39:55Z

braindecode/datautil/windowers.py

+        if (stops[-1] + trial_stop_offset_samples >
+                ds.raw.n_times + ds.raw.first_samp):


This seems potentially quite errorprone code, do we have a test for this?

Yes, test_windowers.py::test_windows_cropped used to test this indirectly - I'll add something more specific to this catch.

Actually I'm not 100% sure the behaviour of mne.Epochs is correct when the raw file has been cropped. Maybe there's something I'm missing that would make the handling of raw.first_samp simpler... I opened an issue here mne-tools/mne-python#8201 just in case.

So now this will be correct, but need newest mne from master? just for clarification

robintibor · 2020-08-14T15:44:57Z

braindecode/datautil/windowers.py

+                window_size_samples = stops[0] - (
+                    onsets[0] + trial_start_offset_samples)


here does not seem to account for trial stop samples?

robintibor · 2020-08-14T15:45:14Z

braindecode/datautil/windowers.py

+    This function extracts windows of size window_size_samples in the interval
+    [trial_start_offset_samples, trial_stop_offset_samples] around each event,
+    with a separation of window_stride_samples between consecutive windows. If
+    the last window around an event does not end at trial_stop_offset_samples
+    and drop_last_window is set to False, an additional overlapping window that
+    ends at trial_stop_offset_samples is created.

    in mne: tmin (s)                    trial onset        onset + duration (s)
    trial:  |--------------------------------|--------------------------------|


is doc here maybe wrong/inconsistent?
Below it says:
trial_stop_offset_samples - Stop offset from original trial stop, in samples.
This is also what happens in code in _compute_window_inds:

braindecode/braindecode/datautil/windowers.py

Lines 290 to 294 in fac6cea

starts = np.array([starts]) if isinstance(starts, int) else starts

stops = np.array([stops]) if isinstance(stops, int) else stops

starts += start_offset

stops += stop_offset

To me, "this function extracts windows of size window_size_samples in the interval" reads quite different.

Good point, I'll fix the docstring.

robintibor · 2020-08-14T15:51:29Z

braindecode/datautil/windowers.py

-        stop = ds.raw.n_times if stop_offset_samples == 0 else stop_offset_samples
-        stop = stop - window_size_samples
+        stop = ds.raw.n_times \
+            if stop_offset_samples is None else stop_offset_samples
+        stop = stop - window_size_samples + ds.raw.first_samp


yeah great, not even sure what was logic behind previous decision. seems quite strange. I wonder actually if we had intended stop offset samples to refer to offset to end of recording? as it is in from events thingy and end of trial?

Aah you have a good point, that's probably what was going on. I think the new approach might be more useful: let's say you want to extract windows from 10 minutes of a recording - you can provide the onset and offset as 2 and 12 mins, which is more natural than 2 and calculate length of recording - (2 + 10). What do you think?

I agree that extracting a particular length from start of recording if you don't have events might be what you want in many cases and would also keep like this for this PR. Only question long-term is if we want to have both options (stop given from beginning of recording and from end of recording), one could also do it in a dirty way: if negative, assume it is from end of recording, if positive, assume from start of recording... but as said let's keep in this way (from start of recording) for this PR

robintibor · 2020-08-14T15:52:52Z

braindecode/datautil/xy.py

@@ -69,7 +69,7 @@ def create_from_X_y(
    windows_datasets = create_fixed_length_windows(
        base_datasets,
        start_offset_samples=0,
-        stop_offset_samples=0,
+        stop_offset_samples=None,


I think with warning makes sense for now? unless somebody suggests sth more sophisticated :)

test/unit_tests/datautil/test_windowers.py

robintibor · 2020-09-01T08:37:32Z

Did you see the comments @hubertjb ?

hubertjb · 2020-09-01T20:16:22Z

Did you see the comments @hubertjb ?

@robintibor yes, sorry for the delay! I'm getting back to it today or tomorrow.

- adding test case for out-of-recording windowing

robintibor · 2020-09-07T09:15:13Z

so seems ready to merge from what I see @hubertjb ?

hubertjb · 2020-09-07T13:27:01Z

so seems ready to merge from what I see @hubertjb ?

Almost! I just need to test with the latest MNE as you said - we might indeed need to ask for the latest version until the next version is out.

hubertjb · 2020-09-08T18:57:00Z

@robintibor the latest commit takes the fix in MNE into account.

I guess we should update the requirements to ask for the latest MNE master (or should we wait for an MNE release)? @agramfort

hubertjb · 2020-09-08T21:07:03Z

(The travis fail is because of the MNE version.)

robintibor · 2020-09-09T10:17:18Z

When will this be in pip-installable mne @agramfort ?

agramfort · 2020-09-09T12:15:35Z

@robintibor @hubertjb you can change the CI here to use:

pip install -U https://github.com/mne-tools/mne-python/archive/master.zip

new MNE version should be released on sept 15

robintibor · 2020-09-09T12:31:07Z

ah great then we can also make new braindecode version on pip quickly after. And for now just put as alex said, if you don't know how to do it, i can also have a try to figure it out @hubertjb

hubertjb · 2020-09-17T20:48:30Z

Tests are passing with the latest MNE master on Travis! Should we wait for the upcoming MNE release before merging?

robintibor · 2020-09-18T12:38:06Z

well how is ETA now for MNE release? last was "new MNE version should be released on sept 15" @agramfort ? should be soon then I guess?

agramfort · 2020-09-18T14:19:06Z

it's imminent. Next monday at the latest

robintibor · 2020-09-22T12:14:00Z

so do you want to try go back to MNE from pip since new version is there? @hubertjb then I can merge

hubertjb · 2020-09-22T13:04:48Z

so do you want to try go back to MNE from pip since new version is there? @hubertjb then I can merge

That'd be great @robintibor, thanks! I just reverted the change to the Travis config file, if tests pass we should be good to go.

robintibor · 2020-09-22T14:29:45Z

hm so seems travis fails with old error, any reason for that @hubertjb @agramfort ? Also CircleCI seems quite a bit slower than all previous builds (12m vs 5-8m when looking at builds from this very same PR (https://app.circleci.com/pipelines/github/braindecode/braindecode?branch=pull%2F152) :/ Don't know why that might be

hubertjb · 2020-09-22T20:04:29Z

Weird... could it be a cache thing? https://docs.travis-ci.com/user/caching/#configuration

robintibor · 2020-09-23T10:15:25Z

It seems reason is mne is still at 0.20.8 at https://anaconda.org/conda-forge/mne . When will this change @agramfort ?

agramfort · 2020-09-23T11:45:23Z

I don't maintain mne in conda. Please use pip

…

robintibor · 2020-09-23T12:00:38Z

ok can you move mne to pip in https://github.com/braindecode/braindecode/blob/master/environment.yml @hubertjb ? I guess that should work then?

hubertjb · 2020-09-23T15:43:32Z

@robintibor it worked! :)

robintibor · 2020-09-23T16:08:14Z

great work @hubertjb ! merged!

- fixing windower functions when first_samp is not 0

fac6cea

- adding `epochs_kwargs` to windowing functions - modifying default value of stop_offset_samples to None for consistency - improving docstrings - adding random state argument to create_mne_dummy_raw - adding tests

hubertjb requested review from robintibor and gemeinl August 8, 2020 17:00

agramfort reviewed Aug 12, 2020

View reviewed changes

- passing mne.Epochs keyword arguments explicitly in windowers

7ed52c8

- using sklearn.utils.check_random_state - updating corresponding test

robintibor reviewed Aug 14, 2020

View reviewed changes

robintibor requested changes Sep 1, 2020

View reviewed changes

- improving doc and formatting

a1ec1e5

- adding test case for out-of-recording windowing

hubertjb mentioned this pull request Sep 3, 2020

MRG: Sleep staging example #161

Merged

robintibor approved these changes Sep 7, 2020

View reviewed changes

fixing create_windows_from_events based on MNE fix of issue #8209

3962a3f

using latest mne master on travis

80adb8e

agramfort approved these changes Sep 18, 2020

View reviewed changes

removing installation of mne master for Travis (mne-0.21 out on PyPI)

29732d0

hubertjb added 2 commits September 23, 2020 11:13

installing mne with pip

ff6e8d0

Merge branch 'master' into improved-windowers

68de700

robintibor merged commit 25abd27 into braindecode:master Sep 23, 2020

hubertjb deleted the improved-windowers branch September 23, 2020 19:07

		if (stops[-1] + trial_stop_offset_samples >
		ds.raw.n_times + ds.raw.first_samp):

		window_size_samples = stops[0] - (
		onsets[0] + trial_start_offset_samples)

	starts = np.array([starts]) if isinstance(starts, int) else starts
	stops = np.array([stops]) if isinstance(stops, int) else stops

	starts += start_offset
	stops += stop_offset

Improvements to the windowers #152

Improvements to the windowers #152

Conversation

hubertjb commented Aug 8, 2020

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

agramfort commented Aug 12, 2020 via email

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

robintibor Aug 14, 2020 • 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

robintibor Sep 3, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robintibor commented Sep 1, 2020

hubertjb commented Sep 1, 2020

robintibor commented Sep 7, 2020

hubertjb commented Sep 7, 2020

hubertjb commented Sep 8, 2020

hubertjb commented Sep 8, 2020

robintibor commented Sep 9, 2020

agramfort commented Sep 9, 2020

robintibor commented Sep 9, 2020 • edited Loading

hubertjb commented Sep 17, 2020

robintibor commented Sep 18, 2020

agramfort commented Sep 18, 2020 via email

robintibor commented Sep 22, 2020

hubertjb commented Sep 22, 2020

robintibor commented Sep 22, 2020 • edited Loading

hubertjb commented Sep 22, 2020

robintibor commented Sep 23, 2020

agramfort commented Sep 23, 2020 via email

robintibor commented Sep 23, 2020 • edited Loading

hubertjb commented Sep 23, 2020

robintibor commented Sep 23, 2020

robintibor Aug 14, 2020 •

edited

Loading

robintibor Sep 3, 2020 •

edited

Loading

robintibor commented Sep 9, 2020 •

edited

Loading

robintibor commented Sep 22, 2020 •

edited

Loading

robintibor commented Sep 23, 2020 •

edited

Loading