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

progress bar #2

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

progress bar #2

wants to merge 2 commits into from

Conversation

robbertmijn
Copy link

Include progress bar when doing cluster based permutation tests.
Using tqdm (which should come with MNE)

@smathot
Copy link
Owner

smathot commented Sep 20, 2023

Thanks! A progress bar is useful, but it should not print anything to the standard output by default, and the behavior should also be consistent between functions. So I would accept a revised PR if:

  • The progress bar is only shown when specified through a keyword that this is necessary
  • tqdm is only imported when the progress bar is shown
  • All time-consuming functions show a progress bar

@robbertmijn
Copy link
Author

Progbar is now by default disabled and is only imported and used when progbar=True is passed to lmer_series or lmer_permutation_test (not sure if there are other time consuming functions that would benefit from this)

@robbertmijn
Copy link
Author

ps. do you think this is the part where it might be helpful to build in the possibility for multiprocessing? That would drastically speed up the permutation tests (which now take many hours).

@smathot
Copy link
Owner

smathot commented Sep 22, 2023

This looks almost good, but it is not complete. Things to do:

  • Test all new functionality, which includes in this case the two modified functions both with and without progress bar. Don't submit a PR without testing it.
  • Also update and test lmer_crossvalidation_test()
  • Update the docstrings of the modified functions where applicable.

@smathot
Copy link
Owner

smathot commented Sep 22, 2023

ps. do you think this is the part where it might be helpful to build in the possibility for multiprocessing? That would drastically speed up the permutation tests (which now take many hours).

Potentially yes, but before implementing this, I would first check whether this will actually speed things up. statsmodels already does multiprocessing (but doesn't seem to be using all cores by default).

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.

2 participants