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

WIP: Replace simulation time with progress bar #388

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ntolley
Copy link
Contributor

@ntolley ntolley commented Jul 6, 2021

After messing with #77, I think it'd be great to rethink how simulation time is updated on screen. It gets pretty unwieldy with all the print statements and is particularly rough for jupyter notebooks. I've drafted a pretty quick solution using tqdm
image

This works well for single core simulations, but will require some more thought with MPI/joblib (new progress bars get printed on every update which defeats the purpose).

Let me know what you guys think! If you're ok with this solution (and adding a dependency or new code in externals) then I'll keep at this.


pbar = tqdm(total=neuron_net.net._params['tstop'], unit='ms',
Copy link
Collaborator

Choose a reason for hiding this comment

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

does this actually work on Jupyter notebook? I seem to remember they have a separate function for that. Or has that changed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They have a gui parameter I haven't tested yet, but the attached screenshot is from a jupyter lab without any modification

@jasmainak
Copy link
Collaborator

jasmainak commented Jul 6, 2021

I think tqdm is great but I have one concern. The incremental output in a progressbar depends on the characters for carriage return and newline (\r and \n) working properly. I know some terminals (like for some CIs) don't support this and it can lead to an overly verbose output (the opposite of the intended effect)

It might be worth having verbosity levels first. In MNE, you can have 'info', 'debug', 'warning' etc as verbose levels. In scikit-learn, verbose can be an int. A very simple start might be to add verbose=True or verbose=False. It might already solve some of your issues ...

Take a look here: https://docs.python.org/3/howto/logging.html

@ntolley
Copy link
Contributor Author

ntolley commented Jul 6, 2021

Great suggestion, I'll start with that. Let's push this to 0.3, mainly opened this PR as a reminder once I finish my 0.2 work.

@jasmainak
Copy link
Collaborator

@ntolley I think it would be good to revive this PR. I was thinking that this would be useful as well. Maybe tqdm should be an optional dependency though

@ntolley
Copy link
Contributor Author

ntolley commented Mar 26, 2022

Played around with this a bit more but it's not clear to me how to resolve the printing issue in the MPI case. Are you guys aware of any packages that print progress bars in parallel applications?

@jasmainak
Copy link
Collaborator

jasmainak commented Mar 26, 2022 via email

@jasmainak
Copy link
Collaborator

@ntolley looking at your jupyter-notebook on the alpha-beta tutorial made me come back to this PR. Do you think you could rebase so I can check how the doc builds look?

@@ -69,8 +70,10 @@ def _simulate_single_trial(net, tstop, dt, trial_idx):
h.finitialize()

def simulation_time():
print('Simulation time: {0} ms...'.format(round(h.t, 2)))
pbar.update(10)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
pbar.update(10)
if check_library('tqdm'):
pbar.update(10)
else:
print('.', end='')
sys.stdout.flush()

copy this logic into check.py: https://github.com/mne-tools/mne-python/blob/main/mne/utils/check.py#L105-L108

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