mne-tools / mne-python

MNE: Magnetoencephalography (MEG) and Electroencephalography (EEG) in Python
https://mne.tools
BSD 3-Clause "New" or "Revised" License
2.69k stars 1.31k forks source link

API: Unify lcmv and dics code #5109

Closed larsoner closed 6 years ago

larsoner commented 6 years ago

Some of the inversion / pinv / eigh calls should be unified in the LCMV and beamformer code, see XXX's introduced in _lcmv.py and _dics.py in #5066.

britta-wstnr commented 6 years ago

I'd be happy to take a look next week - or should is this a milestone for 0.16, @larsoner ?

larsoner commented 6 years ago

I don't think it should be a blocker for the release, but if we can get it in that would be great

britta-wstnr commented 6 years ago

Okay, I will try to get a list of what would be needed ASAP - then maybe we can see how big of a project this will be and if it's reasonable to get it done fast?

larsoner commented 6 years ago

Sounds good to me

britta-wstnr commented 6 years ago

As far as I see it, the following needs to be done:

The second point is a bit sensitive, since there are some functionalities that are shared between DISC and LCMV and some that work quite differently.

For a first iteration I would keep everything as it is at the moment and only unify truly shared code, but still making it one file. From there we can see where we want to change the respective methods lcmv and dics. This would not require and API changes.

I am happy to give it a go - if you are okay with this setup @larsoner I would start working on it!

larsoner commented 6 years ago

Yes sounds good to me as two separate PRs. I'm theory no tests should need to change just internal calls.

I would also just put all shared code (including that for the first PR) in the _compute_beamformer.py file for simplicity.

britta-wstnr commented 6 years ago

Okay then I will start with the first one, which should be quick enough because it's all functions, so it's just changing imports and think about how to do number 2 in the meanwhile :wink: PR(s) follow!

larsoner commented 6 years ago

Subsumed by #5365