-
Notifications
You must be signed in to change notification settings - Fork 1.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
WIP: single class to represent multidimensional data #352
Conversation
base on tmin, tstep, nsteps
- __repr__, indexing: only use name if not None - subdata(): index dim with index (not arg) - RM "properties" kwarg
use dimensons' dimrepr
Allow accessig single dimension data with a string
What exactly were your worries? I tried to keep NdVar as un-complex as possible (e.g., the numpy array is always directly accessible at the .x attribute, and accessed form there when NdVar operations are performed). Limiting ourselves to a few dimension types that are relevant for mne should also keep it simpler. |
I appreciate the input but I have to agree with @agramfort; I feel that adding this adds a lot of complexity which is something we should avoid. For M/EEG there are not that many different cases, which IMHO would justify the use of an NdVar object with named dimensions. Usually, the first dimension is space (sensor or source) and the second time. For time-freq we have a third dimension, which is frequency. Right now, it is very clear how the underlying data is organized, so a user could just do In general, I think it is better to use e.g. a dict to hold stc's for different conditions, then you can e.g. use |
There is sensor and source space data with time, frequency and trial/case, that already makes a large number of possible combinations. Something like the NdVar would make handling of these dimensions consistent. Currently for example when you do single trial stcs you just get a list of stcs, which I think is not very useful. IMO there should be a consistent way of handling those axes, even if not through an NdVar.
The user could in fact still use
With single trial stcs this is not a good option. Also, as I illustrated with the Again, I think it would be good to have a more consistent way of handling axes, even if not through an NdVar objects. Do people agree on this? |
Hi @christianmbrodbeck , thanks for preparing this. As impressed as I am by your NdVar object I'm somewhat reluctant supporting it for mne for similar reasons as brought forward by @agramfort and @mluessi. My main point is that it seems easier to me to convey to newcomers what we are doing here if we keep it more flat. Even if it might be less elegant than what you practice, I think its still easier to deal with. Who already has seen a nunmpy array will easily find his way though our data structures which are already enough to learn, I guess. Also it implies less object magic (@agramfort, wow, I'm getting old....). I agree that we should more explore the use of dicts to support labeled access and stuff. Where exactly do you see the biggest inconsistencies with reagrd to indexing and accessing data you would like to improve. How can we get there? |
Here are some points I've been thinking about:
|
I've been thinking a few times about an stc object for epochs in source space ... valid point.
Yes. This is presumably minor and very doable, and certainly a good idea.
Mhmm. This does not sound like something that would look very pythonic using square brackets. Maybe getters could do the job.
This does not sound like something we could get done by 0.6 wdyt? Too many internals to consider ...
We do have support for math in epochs, evokeds and stcs ... But it seems that's not enough?
I think the clustering is not such a valid example here, it's quite abstract I would say. But I agree that the requirements could be made more explicit. But see #590 and the discussion on #580. |
I wonder if it would be feasible to merge Epochs and Evoked objects into a single class that represents sensor space data (and allows multiple averages in an Evoked), and then make an analogous class for source estimates?
How would you approach that? My solution was that the objects keep information about dimensions -- i.e. a topography is like an Epochs object without the time dimension.
Agree, methods seem to be more natural for this than direct indexing
What I had in mind was extending that math to objects with different dimensions, in the way that baseline correction is subtracting a topography that has no time dimension from an Epochs object. It's not a necessity, but I think it would be nice to be able to do something like
Yes that works too, just seems to require somewhat more maintenance especially if now new subclasses arise for which some methods might break... |
sounds reasonable to me. But that would probably break APIs + user code ... @agramfort will love it.
It sounds like this like we first get the evoked topo plot PR merged to be able to more concretely discuss these other points mentioned. The topo case might be a good model candidate to expose these points without scaring object-phobic people ;-) |
haha yeah, but might be minimal if the new object's behavior depends on whether it has a "trial" dimension or not
That would be great, since source space data can be huge. What do you have in mind? An stc object that references Epochs in the same way that Epochs can reference a Raw?
Actually, working on the topomap PR was part of the reason I am coming back to this now ... with the ndvars I had a sensors dimension and could plot any time slice with a one liner, whereas for the PR I had to handle all those arguments again... |
You mean just add a trial dimension to evoked? Mhm, but still the epochs api is a bit different from the evoked. I guess nothing prevents you from dummy-initializing an Epochs object and filling it wich 'trials' of evoked responses ...
Yes, exactly. A subclass of Epochs that takes an inverse operater on init and then computes the source trials on demand + a 'preload' that also allows us to compute the single-trial stcs in one stel and keep them in memory. stc_epochs = SourceEstimatesEpochs(epochs=epochs, inverse_func=lcmv_epochs, preload=False)
for stc in stc_epochs:
stc.data.max() Mhm. but this is something we can already do without such a constructor. I think this would only be of additional value if we could clarify any Epochs levels methods that operate on the single trial stcs. Does that makes any sense? At least stc_epochs.average() would be one, e.g. in the TFR context. Second getitem would be cool so we can handle multiple conditions in source space. Wdyt? More pros to add? Any cons? | cc @mluessi @Eric89GXL @agramfort
Than this seems like being your PR ;-) |
sorry for getting back to this from the start
true and to make this work we need an object that is a list of stc. I would do: apply_inverse_epochs(epochs[[1,5,6]], …) or [stcs[k] for k in [1, 5, 6]] it's easy, requires no code and everybody can write and read it.
evoked.plot_topomap(time) is what I have in mind. It's true that we could have plot_topomap that
that's a debate if we want to use pythonic getitem or a custom method.
let's agree on an interface and use duck typing.
the difficulty with this is that you need to make sure that the same
can you tell where you think we have bad code duplication in the stats |
To take up discussion from #309.