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

Add support for epoch callbacks #1873

Merged
merged 7 commits into from
May 5, 2022

Conversation

bcumming
Copy link
Member

Adds support for the user to add a generic callback that is to be called:

  1. before the first time step
  2. at the end of every epoch

One implementation of such a callback that prints a progress bar to the screen is provided, and wrapped for the Python front end. Example output of the progress bar:

 57% [||||||||||||||||||||||||||||                      ]           575ms

Fixes #1870

@bcumming bcumming requested review from schmitts and jlubo March 25, 2022 17:01
@bcumming bcumming mentioned this pull request Mar 25, 2022
Copy link
Collaborator

@jlubo jlubo left a comment

Choose a reason for hiding this comment

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

The callback and the Python wrapper seem to work well, and the progress bar looks fine!

See below for two minor suggestions. Also, documentation is still missing.

int lpad = percentage * PBWIDTH;
int rpad = PBWIDTH - lpad;
printf("\r%3d%% [%.*s%*s] %12ums", val, lpad, PBSTR, rpad, "", (unsigned)t);
fflush(stdout);
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
fflush(stdout);
if (t == tfinal) {
printf("\n");
}
fflush(stdout);

Automatically adding a linebreak after the simulation has finished.

Copy link
Member Author

Choose a reason for hiding this comment

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

👍

I tweaked it to print a new line and reset the counter so that code like this:

sim.run(1000, 0.1)
sim.run(2000, 0.1)
sim.run(3000, 0.1)

will generate the following output:

100% |--------------------------------------------------|          1000ms
100% |--------------------------------------------------|          2000ms
100% |--------------------------------------------------|          3000ms

sim.run(100)
print('Simulation finished')
print('\nSimulation finished')
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
print('\nSimulation finished')
print('Simulation finished')

Linebreak should have been added automatically (see above).

Comment on lines 583 to 584
const char* PBSTR = "||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||";
unsigned PBWIDTH = 50;
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
const char* PBSTR = "||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||";
unsigned PBWIDTH = 50;
unsigned PBWIDTH = 50;
const std::string PBSTR = std::string(2*PBWIDTH, '|');

Using the "repeat character" constructor of std::string.

Copy link
Member Author

Choose a reason for hiding this comment

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

The benefit of const char* is that the buffer doesn't have to be initialized each time the callback is made, whereas a std::string requires memory allocation on every callback (at least until we can have constexpr std::string).
Instead I used static const std::string that will only be initialized once.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Seems like the best solution!

int val = percentage * 100;
int lpad = percentage * PBWIDTH;
int rpad = PBWIDTH - lpad;
printf("\r%3d%% [%.*s%*s] %12ums", val, lpad, PBSTR, rpad, "", (unsigned)t);
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
printf("\r%3d%% [%.*s%*s] %12ums", val, lpad, PBSTR, rpad, "", (unsigned)t);
printf("\r%3d%% [%.*s%*s] %12ums", val, lpad, PBSTR.c_str(), rpad, "", (unsigned)t);

I had forgot to mention this: the c_str() conversion also has to be used if the std::string is used (see above).

Copy link
Contributor

@thorstenhater thorstenhater left a comment

Choose a reason for hiding this comment

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

One tiny comment, rest is good.

@@ -558,10 +565,47 @@ void simulation::set_local_spike_callback(spike_export_function export_callback)
impl_->local_export_callback_ = std::move(export_callback);
}

void simulation::set_epoch_callback(epoch_function epoch_callback, bool global) {
Copy link
Contributor

Choose a reason for hiding this comment

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

global is redundant? Or is this compatibility with spike_callback?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point. That is a hang over from an earlier draft of the epoch callback.
I realised that the global information is redundant, and the caller can decide whether to attach a callback on one or more distributed contexts for themselves, e.g.:
https://github.com/arbor-sim/arbor/pull/1873/files#diff-5827156d5c4d2c924b0def9bbf4e51db85e1ea4a0c45c75328ca38dab6ea8e12R190

Copy link
Contributor

@thorstenhater thorstenhater left a comment

Choose a reason for hiding this comment

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

👍

@thorstenhater thorstenhater merged commit c5b1436 into arbor-sim:master May 5, 2022
@bcumming bcumming deleted the progress-banner branch June 27, 2022 19:15
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.

Progress measure
3 participants