removed info = _simplify_info(info) from make_lcmv

[neurosignal]

it removes the the maxfilter processing_history from info which leads to incorrect rank detection with MaxFiltered FIFF MEG data.

Thanks for contributing a pull request! Please make sure you have read the
contribution guidelines
before submitting.

Please be aware that we are a loose team of volunteers so patience is
necessary. Assistance handling other issues is very welcome. We value
all user contributions, no matter how minor they are. If we are slow to
review, either the pull request needs some benchmarking, tinkering,
convincing, etc. or more likely the reviewers are simply busy. In either
case, we ask for your understanding during the review process.

Again, thanks for contributing!

Reference issue

Example: Fixes #1234.

What does this implement/fix?

Explain your changes.

Additional information

Any additional information you think is important.

[welcome]

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

[agramfort]

@britta-wstnr do you mind having a look?

[drammock]

cross-ref to forum post: https://mne.discourse.group/t/problem-with-correct-rank-detection-in-make-lcmv-due-to-simplify-info/6138

[larsoner]

The _simplify_info is often there for speed or storage reasons. If we removed it for these, then I think we can do the removal after rank computation and things should be okay. IIRC the rank should end up being stored in the returned filters so we shouldn't need the MF information anymore...

[wmvanvliet]

Nice catch! Yeah, just moving the _simplify_info() down until after the rank computations sounds very reasonable to me.

[neurosignal]

@larsoner @wmvanvliet
Yes, the rank is stored in the computed filter. simplify_info is good to speed up and space saving in other routines; however, when I test with full and simplified info I hardly any difference in time (< 1s) in filter computation. I quickly tried to dig down into the code and realized the no major step is iterated over all the entries of info. So removal or droping down of simplify_info() after rank computation sould equally work.

[larsoner]

I quickly tried to dig down into the code and realized the no major step is iterated over all the entries of info.

It probably looks this way, but you might be surprised. For example, when writing to disk, all entries will be iterated over. It also happens any time there is a deepcopy operation, which happens more often than you might think.

Those simplify-info calls tended to be added only when necessary, so I'd really rather not remove them unless absolutely necessary. It's very likely (almost guaranteed) to cause a performance regression of some sort.

[britta-wstnr]

Hi @neurosignal, thanks for raising this!
It weirdly looks like our maxfiltered data test now breaks:
RuntimeWarning: Something went wrong in the data-driven estimation of the data rank as it exceeds the theoretical rank from the info (102 > 71). Consider setting rank to "auto" or setting it explicitly as an integer.
Can you have a look at that? In the LCMV tests, we do test all rank options on maxfiltered data, as far as I can see:
https://github.com/mne-tools/mne-python/blob/main/mne/beamformer/tests/test_lcmv.py#L789 (this is also the test that fails)

[neurosignal]

Hi @britta-wstnr
Sorry I missed your msg.
BTW, just after I noticed the problem, I tested it by exclusively giving the rank from maxfilter history as rank=raw.info['proc_history'][0]['max_info']['sss_info']['nfree'], but the rank argument in make_lcmv nowadays does not accept an integer, which earlier used to do so. However, the recent versions accept it as a dictionary, and one has to give it as  rank=dict(meg=raw.info['proc_history'][0]['max_info']['sss_info']['nfree']), and this way it works correctly. However, most of the users do not use it like this; they rather use the default one, i.e., info which is supposed to pull the rank correctly. And I think that was the idea behind introducing the info argument sometimes after v0.18. However, the simplified information lost the history and did not serve the intended purpose.

I'll recheck the test you have pointed here and let you know very soon. Thanks.

[jhouck]

Chiming in from https://mne.discourse.group/t/unexpectedly-high-rank-reported-in-make-lcmv/6714 (thanks @hoechenberger!). FWIW the test @britta-wstnr linked above currently runs fine for me without error.

One option would be to make _simplify_info less simple, adding some basic part of the SSS processing history. This might be overkill if make_lcmv is the only use case for the additional information. Another would be to do as @neurosignal implies: test for the presence of the SSS processing history, and if it exists, run compute_rank with something like rank={'meg': info['proc_history'][-1]['max_info']['sss_info']['nfree'] - len(data_cov['projs'])}. For example:

if 'proc_history' in info:
     rank = {'meg': info['proc_history'][-1]['max_info']['sss_info']['nfree'] - len(data_cov['projs'])}

as a hack to my production copy of _lcmv.py, right before _simplify_info, results in the expected rank. However I do still get a warning about a non positive semidefinite data covariance matrix. My data doesn't have EEG so this is relatively simple.

[larsoner]

openmeeg/openmeeg#577

Sure with a default of remove_proc_histoty=True that we set to False in beamforming code would work

Another would be to do as @neurosignal implies: test for the presence of the SSS processing history

This should already happen if the history is there and you do rank='info'

[jhouck]

openmeeg/openmeeg#577

Sure with a default of remove_proc_histoty=True that we set to False in beamforming code would work

I like this better than the other solution.

Another would be to do as @neurosignal implies: test for the presence of the SSS processing history

This should already happen if the history is there and you do rank='info'

It would, but in make_lcmv _simplify_info is called before compute_rank.

[larsoner]

openmeeg/openmeeg#577

Heh, copy-paste error by me here :) I meant to say that your solution of modifying _simplify_info would work...

@neurosignal do you want to try modifying _simplify_info(..., *, remove_proc_history=True) but using remove_proc_history=False in make_lcmv?

[jhouck]

Thanks @larsoner et al!