mne-python icon indicating copy to clipboard operation
mne-python copied to clipboard

group level cluster permutation WIP

Open SophieHerbst opened this issue 3 years ago • 12 comments

SophieHerbst avatar Jun 27 '22 15:06 SophieHerbst

crossrefs:

  • roadmap: https://mne.tools/dev/overview/roadmap.html#clustering-statistics-api
  • big discussion: #4859

drammock avatar Jun 28 '22 13:06 drammock

It sounds like a goal is to make the stats clustering functions more user-friendly. As @drammock points out above, we've had a lot of conversations about this but little progress, so it would be nice to at least get something working!

Given the extent of the conversations above, it seems like it will be difficult to get a one-size-fits-all API settled that everyone can agree on and implemented in a week. From quick chats with @agramfort and @drammock we came up with the idea of putting this first in mne-sandbox, which is meant for experimental code. This allows us to start very small with something that hopefully works well and solves a relevant use case.

Then from there, later we can expand to other inputs with other dimensionality (frequency? 3D space? source labels with neighbor connectivity?), and other stats tests. And as people use it and we modify / update, eventually we can move it to MNE-Python. Thoughts?

larsoner avatar Jun 28 '22 15:06 larsoner

@larsoner I don't know what mne-sandbox is, or how to use it? For now @hoechenberger and I did not touch the stats function used, but we are implementing a wrapper for group statistics that takes evokeds (to be extended to averageTFR) and spits out a results object. The intention being to provide something usable quickly and then make it more universal. Happy to discuss about how to pursue!

SophieHerbst avatar Jun 29 '22 07:06 SophieHerbst

From quick chats with @agramfort and @drammock we came up with the idea of putting this first in mne-sandbox, which is meant for experimental code. This allows us to start very small with something that hopefully works well and solves a relevant use case.

I don't think that's necessary here; we're merely adding one new function and a new class, both of which are very slim. I think marking them as experimental ought to be enough.

I'm worried "burying" these developments in mne-sandbox won't give them the needed exposure to users…

Edit:

Also … the sandbox seems dead; I don't want to use it for my code. Feels like pouring it down the drain???

Screen Shot 2022-06-29 at 11 07 27

hoechenberger avatar Jun 29 '22 09:06 hoechenberger

Worst case, I could imagine creating a separate, tiny package, if it turns out difficult to get this merged into mne:main. Then we can basically do whatever we want 😄 But I'd prefer to get this into mne proper

hoechenberger avatar Jun 29 '22 09:06 hoechenberger

The point is to allow you to iterate quickly before converging on API for a proper mne release. The solution is just temporary

agramfort avatar Jun 29 '22 09:06 agramfort

But isn't a draft PR good enough for this?

hoechenberger avatar Jun 29 '22 09:06 hoechenberger

Don’t expect to see this merged in a few weeks as is

agramfort avatar Jun 29 '22 09:06 agramfort

Worst case, I could imagine creating a separate, tiny package

This seems pretty similar to the idea of putting it in mne-sandbox -- but what is the advantage of a new package? To me it seems worse in that you have the overhead of having to make a new package (repo, setup.py, pypi, conda-forge, etc.). But otherwise very similar (have to install some other package to get the cluster simplifications, it can still be used in an example in MNE-Python, etc.)

I don't think that's necessary here; we're merely adding one new function and a new class, both of which are very slim. I think marking them as experimental ought to be enough.

Yes but from @drammock's links above there have been a lot of discussions about what should be done. Are you sure your approach here satisfies them all? If not, indeed they should be designated as "experimental", and the way we have planned to / wanted to do this so far is by adding things to mne-sandbox. We haven't done this very often, but the purpose of that repo was/is for experimental (or not totally supported) code. So far we have tried not to merge experimental code in MNE-Python itself...

larsoner avatar Jun 29 '22 12:06 larsoner

we're merely adding one new function and a new class, both of which are very slim. I think marking them as experimental ought to be enough.

to make @larsoner's point in a slightly different way: what does it mean to "mark them as experimental"? We don't currently have an established way of doing that within MNE-Python. The established way of doing that is to put it in MNE-Sandbox.

As an aside:

the sandbox seems dead; I don't want to use it for my code. Feels like pouring it down the drain???

Just a few weeks ago someone asked about using my DSS code from one of my old papers, and I was able to point them to the scripts which say from mne_sandbox.preprocessing import dss.

drammock avatar Jun 29 '22 13:06 drammock

Just a few weeks ago someone asked about using my DSS code from one of my old papers, and I was able to point them to the scripts which say from mne_sandbox.preprocessing import dss.

But if we're putting it in a separate repo / package anyway, I might as well create my own one where I'm not restricted by MNE conventions. I don't see the point of the sandbox, sorry.

And it appears that your code from back then still lives in the sandbox and never made it into mne:main, then? Which would be another strong argument for me for not putting anything in there, ever. If it just sits there to … decay.

But maybe my understanding of the sandbox is wrong? What's the procedure to incubate stuff and finally get it merged upstream into "MNE proper"?

hoechenberger avatar Jun 29 '22 14:06 hoechenberger

I just had a video call with Dan and I'm feeling more comfy with mne-sandbox now!

hoechenberger avatar Jun 29 '22 14:06 hoechenberger

We're continuing this at https://github.com/mne-tools/mne-sandbox/pull/31

hoechenberger avatar Sep 02 '22 09:09 hoechenberger