Use locking/unlocking for Info.update() by mscheltienne · Pull Request #9914 · mne-tools/mne-python

mscheltienne · 2021-10-28T13:57:45Z

As discussed here #9867 (comment) I added support for the lock/unlock state in Info.update().

However, this does come with a performance penalty. Consider this simplified Info:

Simplified Info

import contextlib


class Info(dict):
    
    _attributes = {
        'arg1': 'no touch.',
        'arg2': 'this one neither',
        'arg3': lambda x: x}
    
    def __init__(self, *args, **kwargs):
        self._unlocked = True
        super().__init__(*args, **kwargs)
        self._unlocked = False
    
    def __setitem__(self, key, val):
        unlocked = getattr(self, '_unlocked', True)
        if key in self._attributes:
            if isinstance(self._attributes[key], str):
                if not unlocked:
                    pass
            else:
                val = self._attributes[key](val)  # attribute checker function
        else:
            pass
        super().__setitem__(key, val)
    
    @contextlib.contextmanager
    def _unlock(self):
        state = self._unlocked if hasattr(self, '_unlocked') else False
        self._unlocked = True
        try:
            yield
        except Exception:
            raise
        finally:
            self._unlocked = state
            
    def update(self, other=None, **kwargs):
        if other is not None:
            for k, v in other.items():
                self[k] = v
        for k, v in kwargs.items():
            self[k] = v

With the default update method:

%timeit test_update()
3.23 µs ± 12.9 ns per loop (mean ± std. dev. of 7 runs, 100000 loops each)

With the new update method looping on the input and using __setitem__:

%timeit test_update()
4.98 µs ± 43.7 ns per loop (mean ± std. dev. of 7 runs, 100000 loops each)

Do you have any better idea?

drammock · 2021-10-28T14:17:30Z

the performance hit is 1.75 microseconds, I think we can live with that

mne/io/meas_info.py

…f Mapping. [circle full]

larsoner

LGTM +1 for merge once CIs come back happy

larsoner · 2021-10-28T18:02:35Z

Looks like there is one problem in /home/circleci/project/tutorials/simulation/80_dics.py, feel free to fix that one. Then you don't need to push with [circle full] -- CircleCI will see that the example has changed and just run that one example (it looks at the diff and decides what to run automagically if you don't tell it to do a full build).

mscheltienne · 2021-10-28T18:03:15Z

@larsoner Dammit... I pushed the fix before reading the message..

larsoner · 2021-10-28T18:03:44Z

Just do a a git commit --allow-empty -m 'Dont run all' && git push

larsoner · 2021-10-28T18:04:04Z

... or even better, git commit --allow-empty -m 'Dont run all [skip actions] [skip azp]' && git push

larsoner · 2021-10-28T18:04:38Z

(All of our CIs auto-cancel existing runs when new commits show up)

mscheltienne · 2021-10-28T18:12:37Z

So I did notice that your CIs cancel runs on new commits, but I am not (yet) familiar with git in command lines.. For now, every time I tried, I messed up something 😅
I'm using GitHub Desktop which does not allow for empty commits; and on this PC I'm running Windows and both commands failed to execute.. not sure where git is installed and how to get it recognized by the terminal compared to macOS..
I'm sorry, but I am going to let you cancel the run..
BTW, what are [skip actions] and [skip azp] flags doing?

mscheltienne · 2021-10-28T18:13:42Z

And one more unrelated point, I noticed this:

import os.path as op
import mne
from mne.datasets import sample

data_path = sample.data_path(download=False)
raw_fname = op.join(data_path, 'MEG', 'sample', 'sample_audvis_raw.fif')

raw = mne.io.read_raw(raw_fname)
info = mne.create_info(raw.ch_names, 50, raw.get_channel_types())
info.set_montage(raw.get_montage())

Raises:

ValueError: DigMontage is only a subset of info. There are 1 channel position not present in the DigMontage. The required channels are:

['EEG 053'].

Consider using inst.set_channel_types if these are not EEG channels, or use the on_missing parameter if the channel positions are allowed to be unknown in your analyses.

Guessing this dataset is missing the DigPoint for EEG 053 (set as bad channel). Is this intended?

larsoner · 2021-10-28T18:17:36Z

Guessing this dataset is missing the DigPoint for EEG 053 (set as bad channel). Is this intended?

It shouldn't be. More likely it's that get_montage is only returning channel positions for the good channels, which is a bug.

BTW, what are [skip actions] and [skip azp] flags doing?

Telling GitHub actions and Azure pipelines that they don't need to run. In theory the Azure style checks should run anyway (though I think we have a bug where they don't) and the only thing your commit should have changed was the doc build. I'll run style locally to make sure, and open a PR to make it so that our style checks always run...

larsoner · 2021-10-28T18:49:06Z

Traceback (most recent call last):
  File "/home/circleci/project/tutorials/simulation/80_dics.py", line 249, in <module>
    csd_signal = csd_morlet(epochs['signal'], frequencies=[10])
  File "<decorator-gen-152>", line 24, in csd_morlet
  File "/home/circleci/project/mne/time_frequency/csd.py", line 935, in csd_morlet
    epochs, projs = _prepare_csd(epochs, tmin, tmax, picks, projs)
  File "/home/circleci/project/mne/time_frequency/csd.py", line 1052, in _prepare_csd
    warn('Epochs are not baseline corrected or enough highpass filtered. '
  File "/home/circleci/project/mne/utils/_logging.py", line 365, in warn
    warnings.warn_explicit(
RuntimeWarning: Epochs are not baseline corrected or enough highpass filtered. Cross-spectral density may be inaccurate.

larsoner · 2021-10-28T18:51:30Z

... the easiest thing might be to info = read_raw_fif(...).crop(0, 1).resample(1).info

mscheltienne · 2021-10-28T18:59:58Z

With 50 instead of 1 Hz, but yes.. No way to now easily set the filtering frequencies in an Info instance.

larsoner · 2021-10-28T19:17:01Z

Thanks @mscheltienne !

* upstream/main: API: Deprecate mayavi (mne-tools#9904) MRG: Method get_montage() should return good and bad channels. (mne-tools#9920) Use locking/unlocking for Info.update() (mne-tools#9914)

mscheltienne added 3 commits October 28, 2021 15:13

Add update method using __setitem__.

a8de7ca

FIX: MNE functions using info.update().

b9cded4

FIX: Tests using info.update() [circle full]

035b68a

mscheltienne requested a review from britta-wstnr as a code owner October 28, 2021 13:57

mscheltienne mentioned this pull request Oct 28, 2021

Error checking in Info - setting unsupported keys in mne.Info or mne.Info attributes partially works and doesn't show any warnings #9752

Closed

larsoner reviewed Oct 28, 2021

View reviewed changes

mne/io/meas_info.py Show resolved Hide resolved

FIX: Consider argument other as iterable of key/value pairs instead o…

712751e

…f Mapping. [circle full]

larsoner approved these changes Oct 28, 2021

View reviewed changes

FIX: failing tutorial. [circle full]

1738152

Dont run all [skip azp] [skip actions]

0e6ccec

FIX: tutorial. [skip azp] [skip actions]

0ae0926

mscheltienne mentioned this pull request Oct 28, 2021

MRG: Method get_montage() should return good and bad channels. #9920

Merged

larsoner merged commit dac4f16 into mne-tools:main Oct 28, 2021

mscheltienne deleted the use_locking_for_info_update branch October 28, 2021 20:04

mscheltienne restored the use_locking_for_info_update branch October 29, 2021 15:52

mscheltienne deleted the use_locking_for_info_update branch October 29, 2021 15:53

Uh oh!

Conversation

mscheltienne commented Oct 28, 2021

Uh oh!

drammock commented Oct 28, 2021

Uh oh!

Uh oh!

larsoner left a comment

Choose a reason for hiding this comment

Uh oh!

larsoner commented Oct 28, 2021

Uh oh!

mscheltienne commented Oct 28, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

larsoner commented Oct 28, 2021

Uh oh!

larsoner commented Oct 28, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

larsoner commented Oct 28, 2021

Uh oh!

mscheltienne commented Oct 28, 2021

Uh oh!

mscheltienne commented Oct 28, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

larsoner commented Oct 28, 2021

Uh oh!

larsoner commented Oct 28, 2021

Uh oh!

larsoner commented Oct 28, 2021

Uh oh!

mscheltienne commented Oct 28, 2021

Uh oh!

larsoner commented Oct 28, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

mscheltienne commented Oct 28, 2021 •

edited

Loading

larsoner commented Oct 28, 2021 •

edited

Loading

mscheltienne commented Oct 28, 2021 •

edited

Loading