Skip to content

Conversation

@lroberts36
Copy link
Collaborator

@lroberts36 lroberts36 commented Nov 12, 2025

PR Summary

This PR adds the capability to time groupings of tasks in a task list for profiling. For instance, it was used to create the the by multigrid level timing plot shown below. For an example of using the task list timing, see the unit test.

Additionally, this PR starts on adding some unit tests for the tasking infrastructure itself.
mg_timing.pdf

PR Checklist

  • Code passes cpplint
  • New features are documented.
  • Adds a test for any bugs fixed. Adds tests for new features.
  • Code is formatted
  • Changes are summarized in CHANGELOG.md
  • Change is breaking (API, behavior, ...)
    • Change is additionally added to CHANGELOG.md in the breaking section
    • PR is marked as breaking
    • Short summary API changes at the top of the PR (plus optionally with an automated update/fix script)
  • CI has been triggered on Darwin for performance regression tests.
  • Docs build
  • (@lanl.gov employees) Update copyright on changed files

Copy link
Collaborator

@Yurlungur Yurlungur left a comment

Choose a reason for hiding this comment

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

Looks like some of the new tests you added are failing. Otherwise LGTM.

Comment on lines +98 to +99
PARTHENON_REQUIRE(dict_.count(label) > 0, "Asking for non-existent timing region.");
return dict_[label];
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why not just do this?

Suggested change
PARTHENON_REQUIRE(dict_.count(label) > 0, "Asking for non-existent timing region.");
return dict_[label];
return dict_.at(label);

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I like having the descriptive error message and I think this isn't performance critical.

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.

6 participants