more consistent class instances/types

[drammock]

It is unintuitive and surprising (at least to me and @jasmainak) that:

my_epochs = mne.read_epochs('my_data-epo.fif.gz')
isinstance(my_epochs, mne.Epochs)  # returns False ?!!

It would be nice if mne.epochs.EpochsFIF were a subclass of mne.Epochs so that type checking were more straightforward. Similar argument applies to the various different types of Raw objects. @Eric89GXL has suggested (on Gitter) making _baseEpochs public as one solution, with a caveat in the docstring that it shouldn't be instantiated directly in user code.

[jaeilepp]

I'm wondering how much damage it would cause to rename current _baseEpochs to Epochs (and make it public) and the current Epochs to something else like EpochsRaw. I feel this is how it actually should be.

[jaeilepp]

Of course, now I realize that the way we construct the epochs is through mne.Epochs, so it seems painful.

[jaeilepp]

How do you folks feel about having the construction of epochs being done through a method of raw instance? It would be kind of in line with how we create evoked (epochs.average). So the API would be something like raw.make_epochs(events, event_id, ...). Same for goes for raw.find_events(...).

This way we would work around this issue and be able to eventually rename the _baseEpochs to Epochs.

[jona-sassenhagen]

We discussed this before (a raw.to_epochs method). A few guys (I think @jasmainak , maybe @teonbrooks ..? and I) were very much in favour.

[jasmainak]

@jaeilepp you do not want to reopen old issues :)

what do you think of simply adding an epochs_types = _BaseEpochs (like here)? That way it is clear that it is meant to be used only with isinstance and nothing else. Maybe I'm being naive about something, as API discussion is never simple ;)

[jaeilepp]

That's one option. My suggestion was more of a long term vision of eventually moving towards that direction. There is a logical disconnection of having EpochsFIF that does not inherit Epochs. Also, we don't really have any other classes that are constructed directly by calling the constructor (apart from the array constructors).

[drammock]

There is a logical disconnection of having EpochsFIF that does not inherit Epochs.

This is exactly my view as well. Would it work to make the array constructor serve as the base Epochs class, and have any other epoch classes inherit from it? To me that's the most logical arrangement... The default case is the case where your data are already in numpy format, and the special cases are direct-to-epochs from some other format, saving you the trouble of manually converting to numpy first.

[larsoner]

Would it work to make the array constructor serve as the base Epochs class, and have any other epoch classes inherit from it? To me that's the most logical arrangement...

In general we don't want to force people to have things in memory. EpochsFIF, Epochs, and (maybe?) EpochsKIT all allow on-demand reading, as do all of (or almost all of) our Raw* classes. So in this sense, thinking about objects as thin layers over numpy arrays / data in memory isn't great.

There is also a historical issue in that Epochs(raw, ...) has been the way to construct epochs for a long time. Probably the two most used / most important / core classes in mne are Raw and Epochs. Lots of people's code thus uses this pattern, and asking them to change does not seem worth it to me for such a fundamental object. Sure it's a bit of a misnomer, but so far this seems to cause fairly minor inconveniences.

Can people live with epoch_types? I'd much rather deal with this through documentation and minor changes than an overhaul of our fundamental types.

[agramfort]

+1000 ! or just do isinstance(epochs, _BaseEpochs) as we have a base class for Epochs

[larsoner]

In that case we can rename _BaseEpochs -> BaseEpochs to avoid people needing to use private vars. That should work okay, especially if coupled with some documentation in Epochs and read_epochs about what classes are produced. PR welcome if someone has time.

[drammock]

Honestly epochs_types strikes me as a bit of a band-aid solution... we're not actually dealing with a transition in a dependency's API here (as in the py 2-to-3 transition), we're talking about consistency within mne. That said, I don't have a ready-to-hand idea about how to overhaul the class system without it being a ton of work. But I do believe that it should be done. Maybe next GSoC?

[larsoner]

I am -1 on restructuring it. If I were creating MNE from scratch, I would structure it differently. I agree it's not obvious or consistent. But the downsides of restructuring fundamental classes is not worth it. We have too many users and too much existing code to make the switch.

[drammock]

Lots of people's code thus uses this pattern, and asking them to change does not seem worth it to me for such a fundamental object.

sklearn did something similar a while back, and one of the goals of that change was consistency. In my view these kinds of overhauls are part of the normal pre-version-1.0 throes of software development.

[agramfort]

let's say you have a make_epochs function that takes a raw instance like Epochs constructor now. how will it help you to do type checking?

[drammock]

@agramfort I agree that a function mne.make_epochs(raw, ...) does not address the type checking issue. Neither does a raw.to_epochs method that @jaeilepp suggested. But I definitely don't agree with your suggestion to use the private class and

just do isinstance(epochs, _BaseEpochs)

@Eric89GXL's suggestion to make epochs_types would solve the problem, but has drawbacks that I've mentioned above. The simplest solution to the type-checking problem is making _BaseEpochs public (and adding appropriate warnings if people try to use it directly as a constructor). But the best solution, in my opinion, is to have a base class called Epochs that is used as a constructor, and triages to create an object of one of the subclasses (EpochsArray, EpochsRaw, EpochsFIF, etc) depending on the type of its first argument. All the subclasses inherit from the base class, and the base class is public (for easy type checking) and (as a bonus) the base class gets the most generic name, "Epochs".