DOC: impedances may be obtained for BrainVision

[sappelhoff]

This was not clear from the documentation before.

[adam2392]

LGTM. Would it be worthwhile to mention that it will be None if nothing was read re impedances?

[cbrnr]

LGTM. I don't think mentioning that this attribute is None if no impedance values are available is necessary.

[scott-huberty]

Question: I see that this attribute was discussed and added many years ago in #7974, but I thought that attributes that don't survive I/O roundtrips aren't supposed to be added?

e.g. #11825 (comment)

[larsoner]

I think we made an exception for BV at one point. But thinking about it I think I'd rather deprecate this param and show an example of how to use pybv directly to access this information.

[drammock]

my guess is that .impedances is allowed to deviate for legacy reasons (probably either someone judged that it would break too much user code to remove it, or it's needed by downstream libraries).

edit: hadn't seen Eric's comment until after my review

[mscheltienne]

Note that I actually stumble upon this while adding the ANT reader, and thus I added a similar ìmpedance attribute in the ANT reader. Maybe it should be removed as well and live in antio.

[drammock]

my guess is that .impedances is allowed to deviate for legacy reasons (probably either someone judged that it would break too much user code to remove it, or it's needed by downstream libraries).

edit: hadn't seen Eric's comment until after my review

[sappelhoff]

I think we made an exception for BV at one point. But thinking about it I think I'd rather deprecate this param and show an example of how to use pybv directly to access this information.

I agree in principle @larsoner and I'd be happy to take this functionality out of mne-python and give it a new home in pybv. The order of steps could be:

1. merge this PR as an interim solution
2. move functionality from mne to pybv, still support the attribute in mne but import pybv to get it
3. deprecate the attribute in mne, and write a note in the BrainVision Raw docstr that pybv can be used to obtain impedances

Any other suggestions?

[larsoner]

1. merge this PR as an interim solution
2. move functionality from mne to pybv, still support the attribute in mne but import pybv to get it
3. deprecate the attribute in mne, and write a note in the BrainVision Raw docstr that pybv can be used to obtain impedances

I'm not sure there is a ton of value to (1) or (2) ideally (3) would make (1) and (2) obsolete. But if we don't get around to (3) for a while then (1) is useful in the interim (and done already!), so why not at least merge this.

Note that I actually stumble upon this while adding the ANT reader, and thus I added a similar ìmpedance attribute in the ANT reader. Maybe it should be removed as well and live in antio.

Yes! In fact this makes me think that we should have in a tutorial somewhere about how you can get additional information not provided by mne using these other modules. It could be organized by type of info like impedance, reference schemes, etc. (my preference) or by manufacturer. Can already show pybv and antio as mentioned but also probably mffpy. @sappelhoff or @mscheltienne would either one of you like to get the ball rolling?

[larsoner]

Thanks @sappelhoff !

[sappelhoff]

Can already show pybv and antio as mentioned but also probably mffpy. @sappelhoff or @mscheltienne would either one of you like to get the ball rolling?

I am currently a bit short on time with a big backlog, so I will have to deprioritize this, but I have it on my list for pybv, at least. Would be happy if someone beat me to it though :-)

bids-standard/pybv#122