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 some diagnostics for Parthenon-VIBE #982

Merged
merged 9 commits into from
Dec 12, 2023
Merged

Add some diagnostics for Parthenon-VIBE #982

merged 9 commits into from
Dec 12, 2023

Conversation

Yurlungur
Copy link
Collaborator

@Yurlungur Yurlungur commented Dec 12, 2023

PR Summary

As a way to quickly gut check some measure of correctness, and to highlight the history features of parthenon, I add some tooling to output the total "mean-square mass" of the Burger's variables in Parthenon-VIBE into the history file. I also output the total number of meshblocks at the time of history dump.

This can be compared, up to some tolerance, for various configurations or pieces of hardware by diffing using the burgers_diff.py script that I also add.

In principle, this could be hooked into a gold file for regression testing, but I think for now this is good enough. It provides some useful machinery for us to use for, e.g., machine acceptance criteria.

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

@Yurlungur Yurlungur added the enhancement New feature or request label Dec 12, 2023
@Yurlungur Yurlungur self-assigned this Dec 12, 2023
@Yurlungur Yurlungur enabled auto-merge December 12, 2023 00:47
Copy link
Collaborator

@jdolence jdolence left a comment

Choose a reason for hiding this comment

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

I think this looks good. Thanks!

@Yurlungur Yurlungur requested a review from gshipman December 12, 2023 00:50
Copy link
Collaborator

@pgrete pgrete left a comment

Choose a reason for hiding this comment

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

LGTM (just some minor comments)

Comment on lines +28 to +30
def get_rel_diff(d1, d2):
"Get relative difference between two numpy arrays"
return 2 * np.abs(d1 - d2) / (d1 + d2 + 1e-20)
Copy link
Collaborator

Choose a reason for hiding this comment

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

How about using some of the numpy testing functions instead (which may be helpful wrt to floating point comparisons at different scales)?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This looks cool---I wasn't aware of these. But they all seem to be asserts, whereas I would prefer to print an error message and the location in the array that is off. Is there functionality for that?

Copy link
Collaborator

Choose a reason for hiding this comment

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

There are also the non-testing/assert versions of most of the functionality, e.g., https://numpy.org/doc/stable/reference/generated/numpy.isclose.html

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hmm this still doesn't quite meet needs I think, as it doesn't easily output this diff.

benchmarks/burgers/burgers_diff.py Outdated Show resolved Hide resolved
benchmarks/burgers/burgers_package.cpp Show resolved Hide resolved
benchmarks/burgers/burgers_package.cpp Show resolved Hide resolved
benchmarks/burgers/burgers_package.hpp Show resolved Hide resolved
Copy link
Collaborator

@gshipman gshipman left a comment

Choose a reason for hiding this comment

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

@Yurlungur , looks good. Thx!

@Yurlungur Yurlungur disabled auto-merge December 12, 2023 20:46
@Yurlungur Yurlungur enabled auto-merge December 12, 2023 20:46
@Yurlungur Yurlungur disabled auto-merge December 12, 2023 22:14
@Yurlungur Yurlungur enabled auto-merge December 12, 2023 22:14
@Yurlungur Yurlungur merged commit e8e16b9 into develop Dec 12, 2023
49 checks passed
@Yurlungur Yurlungur deleted the jmm/vibe-diags branch December 12, 2023 23:28
@Yurlungur Yurlungur mentioned this pull request Dec 13, 2023
12 tasks
@gshipman
Copy link
Collaborator

@Yurlungur this looks great. Can you add example usage and update the submodule in the LANL benchmarks site? github.com/lanl/benchmarks

@Yurlungur
Copy link
Collaborator Author

@Yurlungur this looks great. Can you add example usage and update the submodule in the LANL benchmarks site? github.com/lanl/benchmarks

Yes---I'll do that. Thanks for the reminder, @gshipman

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants