MRG: Add a locking/unlocking context manager to Info for error checking.

[mscheltienne]

Following the discussion on #9752 and the proposition in #9767 (comment), here is a possible implementation of @larsoner proposition.
I removed the try/else/finally block, as at least for now, it might be masking potential bugs.

I added the unlocking context manager under the _unlock name; with the current state in a variable named _unlocked.
I added a list of all the valid attributes in Info in the form of the dictionary _attributes. For each valid key, the value is either:

• a string to raise an error message to guide users changing Info attributes without unlocking first towards the correct setter function.
• a function to check if the type/value looks correct before setting for attributes that can directly be set by the user.

I added the corresponding __setitem__ method. I kept the possibility to set non-valid keys with a warning message.

This is obviously not going to pass the CI tests, as the MNE function setting attributes now have to be changed to use the context manager. Is this proposition with a dictionary containing either the error message to raise or the checker function OK for everyone?

---

What are the attributes that can be set directly by the users and that need a quick checker function?

• bads
• description
• experimenter
• subject_info
• device_info
• helium_info

Correct? Any others?

---

For the checker function, is a check of the type as proposed good enough for now, with potential improvements later on?
e.g. for bads we could check that the elements of the list are within the info ch_names.

[larsoner]

I removed the try/else/finally block, as at least for now, it might be masking potential bugs.

Can you give me an example of what you mean?

[mscheltienne]

I added a couple of error messages as examples. Now is a good time to settle on a common syntax.
In the examples, I used:

This attribute can not be directly set. Please use instance methods ... instead.

Maybe replacing instance methods with a more explicit <Raw | Epochs | Evoked> methods would be better.

Note that I do not know what should be the error message for most of them. I do not know which function users are supposed to use, especially for all the MEG-related attributes. Some help on this part would be appreciated.

---

I will start adding the unlocking context manager across MNE functions to get more and more CI tests working again.

[larsoner]

Maybe replacing instance methods with a more explicit <Raw | Epochs | Evoked> methods would be better.

Figuring out which inst an inst.info belongs to is tricky. I would just tell people more specifically to use e.g. "call inst.set_channel_types instead of trying to set inst.info['chs']" or whatever and not worry too much about what inst actually is (I think it's clear enough)

Is dev_head_t an input that can be directly set by the user or not?

There are some circumstances where it can help to set it, but they're pretty rare. I guess for now we say yes

[larsoner]

We have some magic to override deepcopy to speed it up, perhaps it's in there

[mscheltienne]

The copy was an easy one indeed.
I didn't pick up right away that the variable result was an Info instance, not the greatest name 😅
2 tests are still failing in test_meas_info:

• Casting to and from a dictionary:

git\mne-python\mne\io\tests\test_meas_info.py:223: in test_info
    info2 = Info(info_dict)
git\mne-python\mne\io\meas_info.py:678: in __init__
    self['meas_date'] = _ensure_meas_date_none_or_dt(meas_date)
git\mne-python\mne\io\meas_info.py:686: in __setitem__
    raise AttributeError(self._attributes[key])
E   AttributeError: meas_date can not be set directly. Please use method inst.set_meas_date() instead.

• Merge info:

git\mne-python\mne\io\tests\test_meas_info.py:389: in test_merge_info
    info_merged = _merge_info([info_a, info_b])
<decorator-gen-30>:24: in _merge_info
    ???
git\mne-python\mne\io\meas_info.py:2206: in _merge_info
    info[k] = _merge_info_values(infos, k)
git\mne-python\mne\io\meas_info.py:686: in __setitem__
    raise AttributeError(self._attributes[key])
E   AttributeError: acq_pars can not be set directly. See mne.AcqParserFIF() for details.

[hoechenberger]

Okay, down to +844 −422 which has a nice symmetry.

😁😎

[mscheltienne]

That was fast! I wasn't expecting to wake up this morning with CI all green and the diff down again, thanks!

[mscheltienne]

I broke the symmetry, soorry 😅

[...] I think we should also define dict.update with something similar as __setitem__, in case some funny users decide to set attributes this way.

In another PR?

[mscheltienne]

One more bug somewhere, unrelated. I was playing around with the xdawn example and I got this:

tmp = epochs[:1]

repr(epochs)
Out[18]: "<Epochs |  288 events (all good), -0.0998976 - 0.299693 sec, baseline off, ~10.9 MB, data loaded,\n 'Auditory/Left': 72\n 'Auditory/Right': 73\n 'Visual/Left': 73\n 'Visual/Right': 70>"

repr(tmp)
Traceback (most recent call last):

  File "C:\Users\Mathieu\AppData\Local\Temp/ipykernel_12700/3845630344.py", line 1, in <module>
    repr(tmp)

  File "c:\users\mathieu\documents\git\mne-python\mne\epochs.py", line 1650, in __repr__
    s += ', %g - %g sec' % (self.tmin, self.tmax)

  File "c:\users\mathieu\documents\git\mne-python\mne\epochs.py", line 1634, in tmin
    return self.times[0]

IndexError: index 0 is out of bounds for axis 0 with size 0

With:

tmp.times
Out[16]: array([], dtype=float64)

epochs[:1].times
Out[17]: 
array([-0.09989761, -0.09323777, -0.08657793, -0.07991809, -0.07325824,
       -0.0665984 , -0.05993856, -0.05327872, -0.04661888, -0.03995904,
       -0.0332992 , -0.02663936, -0.01997952, -0.01331968, -0.00665984,
        0.        ,  0.00665984,  0.01331968,  0.01997952,  0.02663936,
        0.0332992 ,  0.03995904,  0.04661888,  0.05327872,  0.05993856,
        0.0665984 ,  0.07325824,  0.07991809,  0.08657793,  0.09323777,
        0.09989761,  0.10655745,  0.11321729,  0.11987713,  0.12653697,
        0.13319681,  0.13985665,  0.14651649,  0.15317633,  0.15983617,
        0.16649601,  0.17315585,  0.17981569,  0.18647553,  0.19313537,
        0.19979521,  0.20645505,  0.21311489,  0.21977473,  0.22643457,
        0.23309442,  0.23975426,  0.2464141 ,  0.25307394,  0.25973378,
        0.26639362,  0.27305346,  0.2797133 ,  0.28637314,  0.29303298,
        0.29969282])

Problem is with tmp.resample that empties tmp.times. Is there any better way to get this plot than to resample to 1 Hz?

[mscheltienne]

To build upon this, I thought about defining an info and the montage from the epochs instead of resampling.

- tmp = epochs[:1]
- tmp.resample(1)
+ info = create_info(epochs.ch_names, 1, epochs.get_channel_types())
+ montage = DigMontage(dig=epochs.info['dig'], ch_names=epochs.ch_names)
+ info.set_montage(montage)

But, it's a pain to match the element in epochs.info['dig'] with the channels in epochs.ch_names.
IMO, when creating an epoch instance on a given set of picks, or when picking a set of channels; in this case eeg channels, then info['dig'] should be updated as well and keep only the picked DigPoint.

For another PR as well, right?

[larsoner]

One more bug somewhere, unrelated. I was playing around with the xdawn example and I got this:

I consider this a bug with resample -- it should always yield at least one point. But let's fix that in another PR.

To build upon this, I thought about defining an info and the montage from the epochs instead of resampling.

In principle I think you should be able to use tmp.set_montage(epochs.get_montage()) and have everything work. But this is green so let's play with this in the follow-up PR about resampling.

[...] I think we should also define dict.update with something similar as setitem, in case some funny users decide to set attributes this way.

In another PR?

Argh I had assumed that this would route through __setitem__ but it doesn't. Yes let's do this separately, what's here is already a great start.

@agramfort I think this one is ready to go

[agramfort]

I know a number of projects / code that use info to store temporary stuff that are moved around with the inst. With this PR it's not possible anymore without a warning and knowing about the private _unlock method. What you think of allowing a dedicated key eg "misc" "stuff" "tmp" that user would be free to use without having anything to justify?

[mscheltienne]

I know a number of projects / code that use info to store temporary stuff that are moved around with the inst. With this PR it's not possible anymore without a warning and knowing about the private _unlock method. What you think of allowing a dedicated key eg "misc" "stuff" "tmp" that user would be free to use without having anything to justify?

I don't mind, but I'm not the one to convince #9867 (comment) 😉

With the code in the PR, you get a warning if you define a non-valid key, BUT you don't need to be in an unlocked state to do so. Thus, users don't have to know about the private locking/unlocking functions.

[larsoner]

I can live with 1) one cycle of deprecation warning saying it will be an error in the next release, and 2) add info["temp"] as an explicit temporary (won't survive round trip) option for people going forward.

[mscheltienne]

@larsoner For the deprecation warning, I don't know how you want to set it up. I'm guessing the decorator docs.deprecated is not adapted to this case. I will let you add it as you see fit.

I added the temp attribute, and I put it (for now) last in the docstring. Most of the docstring attribute list is sorted in alphabetical order. In a follow-up PR, I will sort it completely alphabetically, and add the 10 missing attributes.

[agramfort]

Why is that? I think it's better if this variable remains flexible and if users can set it as they want.

+1

[larsoner]

@hoechenberger feel free to take a look at meas_info.py and see if you're happy with the warnings

[larsoner]

Okay let's see how much stuff we break, thanks @mscheltienne !