-
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
new cluster test API #12663
base: main
Are you sure you want to change the base?
new cluster test API #12663
Conversation
for more information, see https://pre-commit.ci
…inaFo/mne-python into new_cluster_stats_api_GSOC24
for more information, see https://pre-commit.ci
…inaFo/mne-python into new_cluster_stats_api_GSOC24
for more information, see https://pre-commit.ci
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just a few comments to get you rolling. Maybe ping us when the formulaic bit is added? (or sooner if you have any questions in the meantime, of course!)
This might be related: |
I think it is a great idea to revamp the current cluster permutation test API 🚀! Could you please share the reasoning behind choosing a pandas DataFrame as the container for evokeds and related meta info? If possible, I'd try to avoid using pandas (an optional dependency) when something similar could be achieved using, for example, a simple dictionary. |
But this doesn't need to be user-facing, then, no? Just trying to understand. If a user passes in a list of TypedDicts or Dataclasses, you can internally create the DataFrames that need to be passed to formulaic. The user would then also get tab-completion assistance in their editor. But it's just a thought. Great work so far in any case! |
This would still require pandas to be available though. |
Sure |
For context, this is step 1 of a GSoC project. A later step involves (probably) creating helper functions that will create the necessary DataFrame for the user. That's not done here because:
|
@CarinaFo I pushed a commit to add the dataset and add If you look at the CI runs, you can see that CircleCI, which builds modified examples/tutorials in PRs, hit an error up on 9c8ec90:
Then I pushed a little commit to fix that in 47363b5, and now it hits a different error (which replicates what I saw locally when I tried to run the example):
It would be good to keep CircleCI green at least so we can see renderings of the tutorial with each push. If you make sure |
81ce0d0
to
6322499
Compare
@CarinaFo FYI I needed to do a rebase and force-push in order to get the CIs to run again, as there was a merge conflict. You'll want to recreate your local branch. Ping me if you have questions / challenges (e.g. if you had local work you don't want to lose) and we can work through it together. |
set up new cluster_test api that sets up design matrix based on Wilkinson formula (using formulaic package)