Skip to content

Update built-in timer documentation #3490

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

Merged
merged 16 commits into from
Jun 27, 2025
Merged

Conversation

jessica-mitchell
Copy link
Contributor

@jessica-mitchell jessica-mitchell commented Apr 30, 2025

This PR adds a diagram describing a simplified simulation run focusing on timers.
The diagram is generated via a script that converts YAML into a dot diagram. Thanks to @JoseJVS for providing the original version.
In addition, some links are added to try to connect various parts of the documentation together with respect to perforance, benchmarking, and timers.

https://nest-simulator--3490.org.readthedocs.build/en/3490/nest_behavior/built-in_timers.html#built-in-timers

@jessica-mitchell jessica-mitchell added T: Enhancement New functionality, model or documentation S: Normal Handle this with default priority I: No breaking change Previously written code will work as before, no one should note anything changing (aside the fix) labels Apr 30, 2025
@github-project-automation github-project-automation bot moved this to In progress in Documentation Apr 30, 2025
Copy link
Contributor

@mlober mlober left a comment

Choose a reason for hiding this comment

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

Thank you for this, it already looks really good. I had a look at the code again and have a few small improvements:

  • diagram: change "Repeat once if buffer too small" to "Repeat once if not all spikes transmitted".
  • diagram: block "if buffer can shrink" must be outside of "Repeat once if not all spikes transmitted".
  • diagram: include another block after time_communicate_spike_data-block that is the opposite of "if buffer can shrink": "if buffer too small -> grow MPI buffer size".
  • OMP parallel and OMP master with same capitalization
  • question to @JanVogelsang : shouldn't the OMP parallel end before OMP master?

@github-project-automation github-project-automation bot moved this from In progress to Review in Documentation Jun 25, 2025
@jessica-mitchell jessica-mitchell requested a review from mlober June 26, 2025 08:08
@JanVogelsang
Copy link
Contributor

JanVogelsang commented Jun 26, 2025

How about making the timer block backgrounds nest-orange instead of white?

shouldn't the OMP parallel end before OMP master?

No that's correct, OMP master sections only make sense within parallel sections, outside of them master is the only thread running anyway

@JanVogelsang
Copy link
Contributor

I'm not sure if "Repeat once if not all spikes transmitted" is actually better than "Repeat once if buffer too small". That makes it sound as if in the first round, the communicated spikes are already used and only the remaining spikes are communicated in the second round.

@mlober
Copy link
Contributor

mlober commented Jun 26, 2025

Keeping it "Repeat once if buffer too small" is fine as well. But I would then write it as "Repeat once if spike buffer too small", since the reader might wonder what this "buffer" is.
One more thing: Jessica and I agreed to remove the blocks about resizing the buffers, since it is not essential to understand the timers and reduces complexity.

@heplesser heplesser merged commit b1e7369 into nest:master Jun 27, 2025
25 checks passed
@github-project-automation github-project-automation bot moved this from Review to Done in Documentation Jun 27, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
I: No breaking change Previously written code will work as before, no one should note anything changing (aside the fix) S: Normal Handle this with default priority T: Enhancement New functionality, model or documentation
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants