API/WIP: new io module

[dengemann]

Just to let you know I'm on it ;-) This addresses our discussion in #383 and the discussion we had while adding BTi and KIT support.
This is still WIP but the fiff module now is deprecated, and you can import mne + the new name space.
Before going on and changing the tests and the examples It would make sense to make sure we converge on the proposed change.

Let me briefly summarize how I wrapped up our discussion so far.

• fiff will be moved to io where it is at the same level as bti and kit, so mne.io.fiff, mne.io.bti, mne.io.kit
• functions related to reading raw files will be available in mne.io, compliant with an read_XXX API
• I hope you're with me with regard to these two last points.
• some important functions, e.g., pick_types, read_evoked, will be linked to the mne name space. Here I would like to poll which functions we would like to see in the mne name space.

[christianbrodbeck]

I have some changes to the kit module in #379 (to provide support for a GUI) and I'm afraid that rebasing will be hell - can we maybe merge those changes separately before changing the mne fiff namespace?

[dengemann]

On 01.04.2013, at 00:17, Christian Brodbeck notifications@github.com wrote:

I have some changes to the kit module in #379 (to provide support for a GUI) and I'm afraid that rebasing will be hell - can we maybe merge those changes separately before changing the mne fiff namespace?

Sure, no hurry. For now i just would like to stimulate a dicussion in which we converge on the details. Also my feeling was that besides a few PRs the time might be good for it, in general.
As far as i see the rebasing efforts should not be too high, thanks to relative imports. It may sound more frightening as it is ;-)

—
Reply to this email directly or view it on GitHub.

[christianbrodbeck]

I meant rebasing changes to files in the kit package, like the RawKIT class which I modified; if the corresponding file moved to a different location, will git know how to apply the changes?

[dengemann]

Ah, ok, got it. Good question. But anyways, I have two other PRs open and my
aim was not to have this one merged asap. Let's close all other urgent /
almost-there PRs and meanwhile focus on the details of API change, would be
my idea.

On Mon, Apr 1, 2013 at 12:34 AM, Christian Brodbeck <
notifications@github.com> wrote:

I meant rebasing changes to files in the kit package, like the RawKIT
class which I modified; if the corresponding file moved to a different
location, will git know how to apply the changes?

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/542#issuecomment-15699174
.

[dengemann]

To add another point we should settle is how to populate the new io name space.
I just removed some (as I think) low level functions from the io name space. The idea would be to have some selected functionality accessible in the io name space while the fiff/ kit /bti name spaces have everything they have. Depending on how this is realized it would implicate some overlap / redundancy. E.g. read_evoked might be available in all three name spaces. I'm not sure whether this is what we want wdyt? To give you an idea, see the snapshots.

[agramfort]

thanks @dengemann for stimulating the discussion on this front.

I think we should delay this PR after the next 0.6 release so we first merge
all WIP PRs.

I feel it's going to be a nightmare to avoid breaking people's code...

[dengemann]

On 03.04.2013, at 09:51, Alexandre Gramfort notifications@github.com wrote:

thanks @dengemann for stimulating the discussion on this front.

I think we should delay this PR after the next 0.6 release so we first merge
all WIP PRs.

Btw. do we want to release 0.6 after all WIPs are merged?

I feel it's going to be a nightmare to avoid breaking people's code...

yes, at least on the long run. I've already experimented with mass-deprecation, see fiff/init.py
which saves at least some typing pain.

—
Reply to this email directly or view it on GitHub.

[agramfort]

I'll have a look when I find the time. It's great if you already have made
proper deprecation.
One way to check that we did not break anything is to run the old test
suite that as no
relative imports after the refactoring.

[dengemann]

One way to check that we did not break anything is to run the old test
suite that as no
relative imports after the refactoring.

yes, given the deprecation is complete + correct, tests should run. Let me
see.

Btw. what would you think about decomposing this into two PRs, one for the
new io module and another for deciding what to add to the mne namespace?
The latter could then become a part of 0.6.

On Wed, Apr 3, 2013 at 9:49 PM, Alexandre Gramfort <notifications@github.com

wrote:

I'll have a look when I find the time. It's great if you already have made
proper deprecation.
One way to check that we did not break anything is to run the old test
suite that as no
relative imports after the refactoring.

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/542#issuecomment-15860095
.

[dengemann]

Btw. old tests pass, and throw deprecation messages.
Here's an excerpt:

denis-a-engemanns-macbook-air-2:mne-python denisaengemann$ nosetests mne/io/fiff/tests/test_raw.py
Test raw copying and appending combinations ... /Users/denisaengemann/python/mne-python/mne/utils.py:229: DeprecationWarning: Class Raw is deprecated; The fiff module will no longer be supported in v0.7. Please use the io module instead.
  warnings.warn(msg, category=DeprecationWarning)
ok
Test raw rank estimation ... /Users/denisaengemann/python/mne-python/mne/utils.py:247: DeprecationWarning: Function pick_types is deprecated; The fiff module will no longer be supported in v0.7. Please use the io module instead.
  warnings.warn(msg, category=DeprecationWarning)
ok
Test saving and loading raw data using multiple formats ... ok
Test loading multiple files simultaneously ... /Users/denisaengemann/python/mne-python/mne/utils.py:247: DeprecationWarning: Function concatenate_raws is deprecated; The fiff module will no longer be supported in v0.7. Please use the io module instead.
  warnings.warn(msg, category=DeprecationWarning)
ok
Test reading/writing of bad channels ... ok
Test IO for raw data (Neuromag + CTF + gz) ... /Users/denisaengemann/python/mne-python/mne/utils.py:247: DeprecationWarning: Function pick_channels is deprecated; The fiff module will no longer be supported in v0.7. Please use the io module instead.
  warnings.warn(msg, category=DeprecationWarning)
...

[dengemann]

... just some maintenance, rebasing + removing k v iterator names from fiff name space (consequence of automated deprecation via iteration over globals).

[bburan-galenea]

@dengemann I've been thinking about modifying the MNE code so that the Raw class can support other file formats such as the one my group uses (ideally there would be an abstract Raw class that all file-format specific Raw classes inherit from). However, it looks like you might already have done some work in this branch. When I looked at it (via the Github interface), I couldn't find the IO module. Did you add the new module and folders to the commit? If so, I apologize, I'm still a bit new to git/github.

[dengemann]

@bburan-galenea Hi Brad, in fact this discussion here is about a planned io module. This PR is pending as we have too many open PRs at the moment this implies a major API change. Probably this won't be merged before mne-python 0.6 is out. In other words the io module only exists on my related branch which I proposed for merge here. However, I fear I have to disappoint you about the new functionality implied. This is basically just about changing names and name-spaces. Your proposal is certainly a good one, however this will demand many wo/man hours. This change would be a major one and setting up all tests, etc. will take even more time than just changing names and their hierarchy.
Currently, the easy way is to make your data compliant with the existing Raw / FIFF API, hence subclassing Raw. Examples can be found in the current master at mne/fiff/bti.py, mne/fiff/kit.py and it shouldn't be too difficult get your data in there. Especially, as after adding kit and bti support we do have some experience with this task.
Just out of curiosity, which system are you using, what does your file format look like?

[bburan-galenea]

@dengemann I work with two file formats. The first is a proprietary one created by MultiChannel Systems and is designed primarily for in vitro and in vivo multichannel electrode recording (arrays implanted in the brain). The second is a HDF5 file (just a lowpass-filtered, downsampled version of the MultiChannel Systems format to speed up data processing).

We don't worry about things such as the inverse solution, sensor geometry, etc. But, we are interested in extracting the event-related LFP/EEG signal from our electrode array (and looking at coherence between electrode pairs). I've primarily been using the code in the time_frequency module, which (for the most part) does not require a Raw/Epoch class. However, many concepts that are currently coded in the Raw/Epoch class appear to be able to address some processing tasks I am currently working on (i.e. filtering, epoch extraction, artifact reject, merging multiple files, etc.). Rather than writing my own code to handle this, it occurred to me that it may make more sense to try to hook into the existing Raw/Epoch framework since it's pretty similar to what I had in mind (albeit a bit more complicated due to the nature of human imaging).

Implementing the Raw API via duck-typing seems reasonable; however, it might be helpful to have a reference interface class (that just spells out the methods/attributes required for minimum functionality and throws a NotImplementedError for methods and assigns NotImplemented to all attributes). This shouldn't require any revision to the tests.

Does this seem reasonable?

[larsoner]

@bburan-galenea this sounds like a reasonable request to me. I've been thinking about doing something similar for some ECoG data that I got my hands on, since mne-python has a lot of tools I'm familiar with. @agramfort @mluessi what do you think about adding a little bit of abstraction so that folks can implement their own Raw / Epochs / Evoked classes more easily?

[dengemann]

@bburan-galenea thanks for the insights, your use case should not be to difficult to realize. We could think about a Raw mixin / ABC that makes the API commitments more explicit. Especially in your case it should not be difficult to meet these requirements, i.e., filling certain fields with dummy info while taking advantage of other methods / reading writing to fiff.

[dengemann]

@Eric89GXL yes. If we plan this carefully it might require changes in reasonable amounts. Besides a minimum API of methods / properties / attributes the reading / writing will certainly be the most challenging part. But if we can set some reasonable defaults for the ABC that should be doable.

[mluessi]

I think making it easier to integrate other file formats etc. is a good idea. Maybe we can something similar as in #382 which introduces a BaseEpochs class. What we have to be careful about is not to introduce too many flavors of "the same" class with slightly different functionality.. this would make testing etc. difficult.

[agramfort]

I like the idea of a BaseRaw object. We need to agree on what is the
interface of Raw instances

• attributes : info (with sfreq, chs, ???), first_samp, last_samp, ch_names
• support raw[:,:] to access data and times
• save ?

I'll let you make this list more exhaustive.

[mluessi]

Maybe it would it make sense to define the interface of the base classes as the one that is used by MNE-Python internally. This would allow any derived class to be passed to functions provided by MNE-Python.

[dengemann]

Wasn't this what this proposal was about?

[bburan-galenea]

In addition to what @agramfort mentioned, a pick_channels method probably would be a good addition since not all datasets have the full complement of channels that MNE typically deals with. This would allow the user to define their own channel categories. Are you suggesting that we implement __getitem__ to support raw[:,:] slicing? Presumably that will be index-based.

[larsoner]

@bburan-galenea you can already do raw[ch_picks, time_picks], see:

https://github.com/mne-tools/mne-python/blob/master/examples/plot_read_and_write_raw_data.py

So in theory, you could just write your own method to come up with ch_picks instead of using mne.fiff.pick_types or mne.fiff.pick_channels.

[bburan-galenea]

@Eric89GXL That's great. Thanks!

[larsoner]

@bburan-galenea do you want to take a stab at enumerating the properties of the abstract class? If you want this to happen on a reasonable timeline, it might be good for you to push for it :)

[bburan-galenea]

I figured that'd be the case :) It may not get done for a few weeks as I've got quite a few other things to take care of. I'll submit a PR once I have something stubbed out for review.

[larsoner]

Great. Once you're going to start working on it, it might be a good idea to post here before doing the coding, in case people have feedback on what should/shouldn't be included. Some people around here can be mighty picky...

[dengemann]

@agramfort now might be a good time to do it. Or directly after releasing 0.6

[agramfort]

@agramfort now might be a good time to do it. Or directly after releasing
0.6

let's do it directly after.

[dengemann]

let's do it directly after.

I think that's a wise idea, let's do it this way.

[christianbrodbeck]

Wait until #379 is merged please... because it adds changes to the kit... I've wanted to add some features but if io should be rearranged soon I can save those for later and get ready for review what I have...

[dengemann]

@christianmbrodbeck --- let me know about your schedule. I really would like to do this soonish before the big 0.7 coding begins. If you are afraid of a mess I'm happy to help you rebasing.

[dengemann]

I figured that'd be the case :) It may not get done for a few weeks as I've got quite a few other things to take care of. I'll submit a PR once I have something stubbed out for review.

ping @bburan-galenea still interested | have time?

[bburan-galenea]

I'm still interested, but at the moment don't have the time to implement this. Sorry! If I get a chance to start working on this in earnest I'll let you guys know.

[larsoner]

@dengemann I have a feeling rebasing this will be nearly impossible, especially after my forward solution PR is merged. I say we close this for now, and once my last two PRs are merged (and the trans-gui one if that's 0.7), then deal with the import parentheticals, then do the IO change. WDYT?

[dengemann]

Keep it open, I'll take care of it then.

[mluessi]

I guess we can remove this from the 0.7 milestone, right?

[dengemann]

@mluessi I think this is the first thing to be done once 0.8 master is open for development. No reason to force this into 0.7 on my end.

[agramfort]

I'd say let's do this right after the release. Any objection? I don't want
to rush anything

[larsoner]

+1 for 0.8.

[dengemann]

Absolutely agreed, already changed this accordingly. I would suggest to discuss and plan the required changes carefully. We don't want to do such big API changes every 12 months ...
fresh 0.8 would be the perfect moment.

[mluessi]

To get the discussion going again.. I was just thinking that it may be better to only have a single class for Raw, Evoked, etc. instead of using subclasses. Basically, the io. module would provide factory functions for each supported type, e.g., the user could use

from mne.io.bti import read_raw
raw = read_raw(fname)

or

from mne.io.fiff import read_raw
raw = read_raw(fname)

in both cases, raw would just be an instance of Raw without any subclassing. This would keep the class hierarchy flat and would make it less bug prone, as all read functions would instantiate Raw with the same constructor. It would also make it easy to support different types of data, e.g., we could add read_evoked to io.bti if there is an evoked format that we can read directly. One problem of this approach may be reading directly from raw files (preload=False) which currently is very fiff specific. So we may still have to use sub classing but we could make sure that all classes call the super constructor.

Thoughts?

[dengemann]

@mluessi I like the functions instead of classes API but I'm not a fan of too deeply nested packages. This is btw. not far from the original proposal to have mne.io.read_raw_bti, mne.io.read_raw_fif. Number of tabs / keystrokes should be the same though ...
Supporting other evoked formats might be interesting, but YAGNI ....

[agramfort]

I like @mluessi's proposal

[larsoner]

I'm fine with either.

[dengemann]

I'm not feeling strongly, I'm happy with Martin's proposal too.

[bburan-galenea]

The advantage of @mluessi's proposal is that it facilitates separation of responsibilities (e.g. one person can primarily be responsible for FIFF, one for BTI, etc.) and having the code for each format in separate modules likely would minimize potential merge conflicts.

[agramfort]

to me this is 1st priority once py3k is working.