search

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

BUG: save beamformer filter #4957

Closed kingjr closed 5 years ago

kingjr commented 6 years ago

Once we have done 

filters = mne.beamformer.make_lcmv()

h5io isnt applicable because of the EmpiricalCovariance object

larsoner commented 6 years ago

Hmm, we shouldn't have an EmpiricalCovariance in there, just Covariance (which is internally a dict and thus h5io-compatible)

britta-wstnr commented 6 years ago

I can put that on my agenda for when I take care of the deprecation of lcmv() (to make it comparable to the DICS API). The question is whether we want to wait for the big DICS PR to be merged first?

larsoner commented 6 years ago

The question is whether we want to wait for the big DICS PR to be merged first?

Yes to avoid having to rebase that one (it's big).

britta-wstnr commented 6 years ago

@larsoner that's what I figured. I will take care of this as soon as the other one is merged then!

larsoner commented 6 years ago

@britta-wstnr it looks like the changes to lcmv.py are pretty small in #5066. Do you have time to tackle saving/loading now? If so we can get probably get it into 0.16, because it should be quite simple with h5io. You can draft a PR for LCMV, we can get that up to snuff while #5066 is merged, then the addition of DICS should be just a dozen or so lines of code hopefully!

britta-wstnr commented 6 years ago

I am confident that I can have a look before next week - also on the list is the deprecation of lcmv() to mimic the new DICS code, I can tackle that in the same go, then.

britta-wstnr commented 6 years ago

So the filter contains the same covariance instance that is fed to make_lcmv (some changes might be done by pick_channels_cov). In my case, this is always an Covariance object, and then everything saves fine. To be able to investigate this: how is the EmpiricalCovariance object obtained?

larsoner commented 6 years ago

To be able to investigate this: how is the EmpiricalCovariance object obtained?

Looking at the code I don't see why it would happen. @kingjr can you provide a snippet to replicate?

In the meantime @britta-wstnr you can proceed as if this is not an issue, assuming we will fix whatever is causing @kingjr to have the an EmpiricalCovariance rather than Covariance.

agramfort commented 6 years ago

@kingjr I cannot replicate.

Putting some of these lines in our tests do not crash:

        # Trying saving to disk
        mne.externals.h5io.write_hdf5('/tmp/tmp.h5', filters, overwrite=True)
larsoner commented 6 years ago

I hit this bug at some point, basically if you compute the cov on the fly it can have cov['estimator'] which is a sklearn object so you need to remove it before saving. I can tackle this I/O for 0.17