Why is `max_ori_out` in `_apply_lcmv` enforced to be "signed"?

[dafrose]

Dear mne developers,

thanks for your great work.

When I looked through the beamformer code in order to find out what the options for the parameter max_ori_out in apply_lcmv are, I found that this parameter is actually enforced to be "signed" with no alternative, while later in the code the option exists for it to be "abs".

First lines of the function: https://github.com/mne-tools/mne-python/blob/c2aaaa8aad83d62ee36b7030bca56d50315020e4/mne/beamformer/_lcmv.py#L203-L207

Later: https://github.com/mne-tools/mne-python/blob/c2aaaa8aad83d62ee36b7030bca56d50315020e4/mne/beamformer/_lcmv.py#L242-L243

It is obvious that one of these two code snippets do not work as intended. What is the intended behaviour?

Kind regards, Daniel

[welcome[bot]]

Hello! 👋 Thanks for opening your first issue here! ❤️ We will try to get back to you soon. 🚴🏽‍♂️

[agramfort]

ping @britta-wstnr

[britta-wstnr]

Hi @dafrose,

as far as I can see, lines 242 and following are a relic of old code, which we sh/could clean up. We took a decision a while ago to not implement abs directly, as this is easy to do on the data afterwards if wished for. IIRC, the parameter is still there not only for backward compatibility, but also because we were thinking about implementing options for vector beamformers (e.g. doing an SVD on the output; it would be a bit of a misnomer there though). Pinging @larsoner Should we remove lines 242ff., what do you think?

Thanks for raising this, @dafrose (and thanks for the ping, @agramfort )

[larsoner]

Agreed we should just get rid of it and people can use abs(stc) if they want unsigned data. @dafrose do you want to make a PR to clean it up?

[dafrose]

Hi @britta-wstnr and @larsoner ,

thanks for the replies. I could submit a PR, question is what needs to be done. Do you mean to just remove the respective lines or is there also documentation that needs to be updated? If you want to keep the max_ori_out parameter for backward compatibility, it should at least be clear from the docs that "signed" is the only option. I'll look through the docs. An alternative would be to at least raise a deprecation warning, when the parameter is used. What do you think about that?

I agree to @britta-wstnr that toggling an SVD for vector beamformers would look strange for this parameter. Why not have a vector-specific parameter or an orientation mode called (e.g.) 'vector-svd' in the first place?

[britta-wstnr]

Hi @dafrose

Great that you want to give it a go! I would start with removing lines 242ff and see if the unit tests are still happy. It is indeed a good idea to reflect this in the documentation (that only 'signed' is possible for now), although I can see that line 272 correctly only lists 'signed'.

Regarding vector beamformers: we have been discussing this before, but IIRC never came to a definite conclusion if this is something that would be used a lot - the rationale being that 1.) users might just run a scalar beamformer (pick_ori='max-power') or 2.) do the needed operation(s) themselves after computing the beamformer output. Is that something we want to discuss again, @larsoner and @wmvanvliet ? I think I am -0.5 for adding this after thinking it through again.

[larsoner]

2.) do the needed operation(s) themselves after computing the beamformer output.

Assuming you're talking about the vector time-courses of shape (n_src, 3, n_times) and doing an SVD on each of the n_src matrices of shape (3, n_times) to obtain a final set of time courses of shape (n_src, n_times), then yes you can just do stc.project('pca'), so no need to make this a beamformer-specific operation as we can think of it really as a type of "post-processing" on the full vector time courses (like stc.magnitude(), or like abs(stc) is for the scalar time courses).

[dafrose]

Would you be fine with setting the default for max_ori_out to None, then

• raise DeprecationWarning, if it is set to 'signed'
• raise ValueError, if set differently (current implementation)

Or would you prefer the default to be 'signed'as-is?

[larsoner]

raise DeprecationWarning, if it is set to 'signed'

Sure, something like this would be fine (note the * to prevent positional args after filters)

def apply_lcmv(evoked, filters, *, max_ori_out=None, verbose=None):
    ...
    if max_ori_out is not None:
        warn('max_ori_out will be removed in 1.0, do not pass it as an argument')
    max_ori_out = 'signed'