Skip to content

Conversation

@christianbrodbeck
Copy link
Member

No public API because @agramfort wants to wait for FASTER to discuss it.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reshaping seems to affect the numbers slightly, I don't know why:

In [14]: x = np.random.normal(0, 1, (50, 50))
In [15]: x2 = np.reshape(x, (1, 50, 50))
In [21]: interp = np.random.normal(0, 1, (1,50))
In [25]: np.array_equal(interp.dot(x), interp[:,None,:].dot(x2)[:,0,0])
Out[25]: False
In [26]: np.allclose(interp.dot(x), interp[:,None,:].dot(x2)[:,0,0])
Out[26]: True

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's possible that the order of operations could differ slightly depending on the striding / array-continuous direction. You could try re-casting both to have the same striding to see if they match. I could also imagine that having continuity in one direction that allowed you to use SSE/AVX SIMD instructions could change the numbers slightly since the summation could effectively be binned differently. Can't remember how strict the linear algebra / implementations are for those operations...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes that sounds reasonable, when I recast the single epoch data I get exact matches (christianbrodbeck@557d9da)

@christianbrodbeck
Copy link
Member Author

All I did was extract the code for creating the goods/bads indexes and morph matrix so they can be cached when applying multiple times.

@christianbrodbeck
Copy link
Member Author

The Appveyor problem seems to be related to downloading the datasets

@larsoner
Copy link
Member

@christianbrodbeck it just occurred to me that an alternative would be to set the values to np.nan for a given channel in a given epoch, then use np.nan* functions down the line. Might that be a better approach?

@dengemann
Copy link
Member

@christianbrodbeck what's up here? Wanna move on?

@christianbrodbeck
Copy link
Member Author

I've implemented it for myself, I thought you did not like the API I suggested?

@dengemann
Copy link
Member

I've implemented it for myself, I thought you did not like the API I suggested?

I think meanwhile everyone agrees tjat we need this stuff!

@christianbrodbeck
Copy link
Member Author

What about the API then? For myself I want the information to be as sparse as possible, because I want to save it along with the raw file and epoching instructions rather than build it into a data object directly. Hence list of lists of channel names seemed the most straight forward to me.

@larsoner
Copy link
Member

@christianbrodbeck do you plan to work on this again? If so, perhaps it should go in the mne-sandbox repo until we settle on the best interpolation/cleaning tools. That way it can get in and used by people instead of sitting idle too long.

@christianbrodbeck
Copy link
Member Author

Well I currently don't have any EEG data but I'm happy to clean it up if we settle on an API.

@larsoner
Copy link
Member

@christianbrodbeck I think FASTER went into mne-sandbox so that might be a good place for this, too. But refactoring the private functions can still be done here if it's useful. I will probably need to refactor them a bit for some upcoming PRs, so it might be good to get those bits in sooner rather than later to avoid some ugly rebasing.

@jasmainak
Copy link
Member

also might be useful to do this for MEG not just EEG interpolation

@christianbrodbeck
Copy link
Member Author

@Eric89GXL it already needs non-straight forward rebasing, I'll leave that for when it actually gets added.

@larsoner
Copy link
Member

@christianbrodbeck are you still interested in seeing / adding this feature? I'm not sure what the overlap is like with autoreject, for example.

@jasmainak what is the overlap like between autoreject and this PR?

@jasmainak
Copy link
Member

There is certainly some overlap. autoreject does epoch-wise interpolation also for MEG data. Although here it looks to me that the caching is implemented more neatly rather than using joblibs as in autoreject.

I'm wondering how much interest there is in terms of having the epoch-wise interpolation + viz for epoch-wise bad channels in MNE. It would be quite helpful in terms of reducing code duplication. Right now, we copy quite a bit of MNE interpolation code.

@agramfort
Copy link
Member

I agree that a (private) API to do epochs wise interp in MNE would be useful.

It should reduce code in autoreject + make comparison of other methods easier (outside of autoreject)

it should work indeed with all channel types.

@christianbrodbeck
Copy link
Member Author

christianbrodbeck commented Aug 13, 2017 via email

@hoechenberger
Copy link
Member

Is this still considered for future inclusion? I would really appreciate this feature.

@agramfort
Copy link
Member

agramfort commented Apr 13, 2018 via email

@mmagnuski
Copy link
Member

Just to bump this issue: I'd go with a dictionary of epoch index -> channel indices or channels -> epoch indices or even an epochs x channels boolean array. The last case is the most intuitive in my opinion although not very memory efficient.

@larsoner
Copy link
Member

A boolean mask would be cleanest to my eye. Memory requirements will stay low enough not to be a concern

@mmagnuski
Copy link
Member

Is the comment about not interpolating meg still the case? If so then if one has eeg + meg the boolean mask would have to contain many Falses for all the meg channels or the mask would only concern eeg channels (or ch_type argument?).
How do you see the GUI interaction to specify channels on epoch-level? modifier + click could be quite cumbersome if the offending channel should be interpolated for many epochs.

@larsoner
Copy link
Member

We can interpolate MEG and EEG data nowadays

@mmagnuski
Copy link
Member

Still ch_type would be useful if one wants to interpolate only EEG.

Base automatically changed from master to main January 23, 2021 18:26
@larsoner
Copy link
Member

Now that #8896 I think there is a path forward for this functionality, but it probably makes sense to start fresh given the changes there and the conflicts here. So I'll close but we can refer to this code if needed when we implement a solution

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants