[MRG+1] Deprecate stim channel synthesis #5712

massich · 2018-11-14T13:32:46Z

Stim channel is synthesized by:

brainvision
edf
eeglab

larsoner · 2018-11-14T15:57:51Z

@massich will you have time for this today? If not, I can do it. It's potentially the last blocker for release

agramfort · 2018-11-14T16:18:07Z

let's see where it brings us. I doubt massich has time today. Maybe we'll merge after the release but it's good to see how much it makes us deprecate lines.

massich · 2018-11-14T16:21:48Z

So far, I've deprecated a private function to see its ripples. These are the test that break.

Results (557.71s):
     890 passed
      19 failed
         - mne/io/brainvision/tests/test_brainvision.py:100 test_vhdr_codepage_ansi
         - mne/io/brainvision/tests/test_brainvision.py:135 test_ascii
         - mne/io/brainvision/tests/test_brainvision.py:175 test_brainvision_data_highpass_filters
         - mne/io/brainvision/tests/test_brainvision.py:233 test_brainvision_data_lowpass_filters
         - mne/io/brainvision/tests/test_brainvision.py:288 test_brainvision_data_partially_disabled_hw_filters
         - mne/io/brainvision/tests/test_brainvision.py:322 test_brainvision_data
         - mne/io/brainvision/tests/test_brainvision.py:423 test_events
         - mne/io/brainvision/tests/test_brainvision.py:595 test_brainvision_with_montage
         - mne/io/brainvision/tests/test_brainvision.py:611 test_brainvision_neuroone_export
         - mne/io/brainvision/tests/test_brainvision.py:643 test_read_raw_brainvision_warn_on_marker_drop
         - mne/io/edf/tests/test_edf.py:59 test_orig_units
         - mne/io/edf/tests/test_edf.py:132 test_edf_data
         - mne/io/edf/tests/test_edf.py:184 test_stim_channel
         - mne/io/edf/tests/test_edf.py:253 test_edf_annotations
         - mne/io/edf/tests/test_edf.py:282 test_edf_stim_channel
         - mne/io/edf/tests/test_edf.py:389 test_find_events_and_events_from_annot_are_the_same
         - mne/io/eeglab/tests/test_eeglab.py:57 test_io_set_raw[fnames0]
         - mne/io/eeglab/tests/test_eeglab.py:57 test_io_set_raw[fnames1]
      37 skipped

What I know so far:

In brainvision, the stim channel is always synthesized at __init__, and its only empty based on event_id. ( I also found that this line only allocates the stim channel and we might want to refactor it )
https://github.com/mne-tools/mne-python/blob/master/mne/io/brainvision/brainvision.py#L170

In edf, we do can control the stim channel, by calling __init__ with stim_channel=None (right now defaults to 'auto').
https://github.com/mne-tools/mne-python/blob/master/mne/io/edf/edf.py#L346

eeglab is really similar to brainvision. The stim channel is always synthetized (it even have the same allocation).
https://github.com/mne-tools/mne-python/blob/master/mne/io/eeglab/eeglab.py#L419

The most conservative approach:

In 0.17:

add a parameter to compute stim_channel for brainvision and eeglab (synthetize_stim_channel=True)

In 0.18:

deprecate synthetize_stim_channel=True for brainvision and eeglab
deprecate stim_channel='auto' default in eeglab in favor of None

In 0.19:

Remove stim_channel

The most agressive approach:

In 0.17:

add a parameter to compute stim_channel for brainvision and eeglab (synthetize_stim_channel=True)
deprecate synthetize_stim_channel=True for brainvision and eeglab
deprecate stim_channel='auto' default in eeglab in favor of None

In 0.18:

Remove stim_channel

larsoner · 2018-11-14T16:33:22Z

I would rather take the aggressive approach. The stim channel synthesis complicates our codebase and provides unsatisfactory solutions for users. We really should move to the annotation->event path ASAP I think

massich · 2018-11-14T17:03:33Z

We were discussing with @agramfort in the blackboard and we thought that the best approach was:

0.17:

brainvison, eeglab: add a parameter stim_channel=True + deprecation warning suggesting to use False
edf: stim_channel='default' + deprecation warning suggesting to use None

0.18:

brainvison, eeglab: stim_channel=False + deprecation warning saying that the parmeter would disspear
edf: if stim_channel is differnt than None warn that the behaviour would disapear

0.19:

Remove synthesis

What you propose as a different timeline?
I mean, if we agree that our users wont mind I'm fine to remove the whole thing in 0.18. The question is how?

larsoner · 2018-11-14T17:32:05Z

To be faster I would do:

0.17:

Add parameter to bv/eeglab: default None (acts like True), say accepted values in docstring are True | False
deprecation warning for all readers that synth will be disabled in 0.18

0.18:

bv/eeglab, the only accepted value in docstring is False (but our default is None, acts like False), if they pass False, warn them to stop passing the parameter
remove synth for all readers, remove param for edf

0.19:

remove param for bv/eeglab

massich · 2018-11-14T18:13:12Z

That works for me. Instead of syncing in 0.17 and then let them die, you let them fade in their own schedule. But it seems that we won't break users code either. That's fine with me. I'm AFK, can you push a proposal in this PR? thx

…

On Wed, Nov 14, 2018, 18:32 Eric Larson ***@***.***> wrote: To be faster I would do: 0.17: - Add parameter to bv/eeglab: default None (acts like True), say accepted values in docstring are True | False - deprecation warning for all readers that synth will be disabled in 0.18 0.18: - bv/eeglab, the only accepted value in docstring is False (but our default is None, acts like False), if they pass False, warn them to stop passing the parameter - remove synth for all readers, remove param for edf 0.19: - remove param for bv/eeglab — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#5712 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AGt-496VES0faHM4f3uCEU1BNJ6t2DGEks5uvFOXgaJpZM4Ydv70> .

agramfort · 2018-11-14T20:08:52Z

@massich and @larsoner you're saying almost the same thing except that @larsoner's proposal allows to deprecated the edf stim channel generation one release earlier. So as long we don't break people's code and we give one release cycle for people to adjust fine with me. @larsoner seems like you're in the starting blocks to do this?

codecov · 2018-11-14T20:31:15Z

Codecov Report

Merging #5712 into master will increase coverage by <.01%.
The diff coverage is 96.66%.

@@            Coverage Diff             @@
##           master    #5712      +/-   ##
==========================================
+ Coverage   88.61%   88.61%   +<.01%     
==========================================
  Files         369      369              
  Lines       69462    69498      +36     
  Branches    11678    11691      +13     
==========================================
+ Hits        61556    61588      +32     
- Misses       5055     5058       +3     
- Partials     2851     2852       +1

larsoner · 2018-11-14T20:31:26Z

Okay plan is laid out in whats_new.rst, hopefully it's clear enough

larsoner · 2018-11-14T20:32:21Z

doc/whats_new.rst

 ~~~

- Deprecate ``mne.io.read_annotations_eeglab`` by `Joan Massich`_
+- Deprecated separate reading of annotations and synthesis of STI014 channels in readers by `Joan Massich`_:


@mmagnuski this is the outline of the changes that would affect read_raw_eeglab and related functions. We should figure out if everything will still work for you!

Here is a more readable version:

https://10219-1301584-gh.circle-artifacts.com/0/html/whats_new.html#api

Thanks, I will try to test on a variety of files during the weekend.

jona-sassenhagen · 2018-11-14T21:58:14Z

mne/io/brainvision/brainvision.py

                dtype_bytes = _fmt_byte_dict[fmt]
                offsets = None
-                n_samples = n_samples // (dtype_bytes * (info['nchan'] - 1))
+                n_samples = n_samples // (dtype_bytes * n_data_ch)


How is this better than before?

Sometimes you need the -1 sometimes you don't

jona-sassenhagen · 2018-11-14T21:59:31Z

mne/io/brainvision/brainvision.py

+                     "that do not follow the BrainVision format. For more "
+                     "information, see the docstring of read_raw_brainvision."
+                     .format(len(dropped), dropped[:5]))
+            self._create_event_ch(events, n_samples)


Might be a good time to refactor and share this with the similar code in the EEGLAB reader.

These lines will be removed as soon as 0.17 is released so it's not worth it

mne/io/brainvision/brainvision.py

mne/io/edf/edf.py

mne/io/utils.py

jona-sassenhagen · 2018-11-14T22:07:58Z

Heavens ...

I will try to look more later.

massich

Looks great! I loved that you used match=foobar to document what should be captured by pytest.deprecated_call.

Thx a lot

massich · 2018-11-15T10:38:12Z

I'll go ahead and merge this one! THX all

massich force-pushed the deprecate_stim_channel_synthesis branch from 434d763 to c7abe15 Compare November 14, 2018 13:41

larsoner added this to the 0.17 milestone Nov 14, 2018

massich mentioned this pull request Nov 14, 2018

MRG: Improve decoding doc and sphinx process #5707

Merged

Break stuff

0c95bc6

FIX: Deprecate stim_channel

909125a

larsoner force-pushed the deprecate_stim_channel_synthesis branch from c7abe15 to 909125a Compare November 14, 2018 20:31

larsoner changed the title ~~[WIP] Deprecate stim channel synthesis~~ [MRG] Deprecate stim channel synthesis Nov 14, 2018

larsoner reviewed Nov 14, 2018

View reviewed changes

larsoner added 2 commits November 14, 2018 16:05

FIX: Remove bad doc

0367425

FIX: Double check BDF

fdaf867