Skip to content
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

group level cluster permutation WIP #10840

Closed

Conversation

SophieHerbst
Copy link
Contributor

No description provided.

@drammock
Copy link
Member

@larsoner
Copy link
Member

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?

@SophieHerbst
Copy link
Contributor Author

@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!

@hoechenberger
Copy link
Member

hoechenberger commented Jun 29, 2022

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
Copy link
Member

hoechenberger commented Jun 29, 2022

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

@agramfort
Copy link
Member

agramfort commented Jun 29, 2022 via email

@hoechenberger
Copy link
Member

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

@agramfort
Copy link
Member

agramfort commented Jun 29, 2022 via email

@larsoner
Copy link
Member

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...

@drammock
Copy link
Member

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.

@hoechenberger
Copy link
Member

hoechenberger commented Jun 29, 2022

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
Copy link
Member

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

@hoechenberger hoechenberger force-pushed the group_level_cluster_test branch from 8a95461 to a2ab296 Compare June 29, 2022 19:09
@hoechenberger
Copy link
Member

We're continuing this at mne-tools/mne-incubator#31

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.

5 participants