-
Notifications
You must be signed in to change notification settings - Fork 191
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 Time-Averaged Field Diagnostics #5285
base: development
Are you sure you want to change the base?
Add Time-Averaged Field Diagnostics #5285
Conversation
Thanks for making this PR! One complication is that with #5176, adaptive timestepping will be possible. This means that the timestep length can change, so simply summing the multifabs and dividing by the period won't be enough for a proper time average. I don't think addressing this will be too hard, however. |
Thanks, @archermarx! I wasn't aware of that yet. 🙂 We can do a follow-up and catch if |
Yep, my thought exactly. The adaptive timestepping PR was just merged, too. |
I think #5296 should fix the clang-tidy errors that remain, which don't seem related to this PR. |
af628fc
to
822034d
Compare
Thanks for the catch! Please add a runtime assert in this PR now. |
0f8c5e0
to
063f151
Compare
Added commit that will cause the sim to abort when The last thing for this PR that remains from our list is the CI tests. @ax3l, should we also use the recently merged |
Just empirically, I tried this in our Laser-Ion Acceleration example. At step 1000, I compared between the electron density output from the in-situ generated averaged output and a post-processed output that I created from 25 instantaneous outputs. The relative difference is on the order of 1e-16. 🎉 Created with the following input script: |
@n01r @RevathiJambunathan |
@EZoni, the CI tests are still missing. I was thinking of adding these today since Perlmutter is under maintenance anyway. |
@@ -20,3 +30,14 @@ add_warpx_test( | |||
diags/diagInst/ # output | |||
OFF # dependency | |||
) | |||
|
|||
# We do not have an intervals parser for PICMI yet which would accept more than a single integer for the output period |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should think about implementing an intervals parser for our PICMI diagnostics which can handle more than a single integer. It should be able to do the same as our app input: https://warpx.readthedocs.io/en/latest/usage/parameters.html#intervals-parser
- Implement warnings and errors for certain inputs concerning averaging periods
- added first documentation on time averaged diags - put more operations on summation multifabs into if-environments
- added time averaged diags to laser-ion acceleration example - fix first issues that came up when testing this
for more information, see https://pre-commit.ci
Currently, we do not yet support the newly implemented adaptive time-stepping mode of electrostatic solvers together with time-averaged diagnostics.
Even though the laser ion test is named as a dependency it is running this test again. That should ideally be avoided. It would be good to just run the analysis script as test.
bf71c29
to
171ce3c
Compare
for more information, see https://pre-commit.ci
@EZoni, feel free to review :) |
For PICMI, the TimeAveragedDiag test is automatically disabled because we cannot simply define the necessary output intervals. We need to be able to define period=[100,69:100] like for the app input.
5979cc4
to
22bc9a9
Compare
for more information, see https://pre-commit.ci
def analyze_openpmd_regression(output_file): | ||
# Run checksum regression test | ||
if re.search("single_precision", output_file): | ||
evaluate_checksum( | ||
test_name=test_name, | ||
output_file=output_file, | ||
output_format="openpmd", | ||
rtol=2e-6, | ||
) | ||
else: | ||
evaluate_checksum( | ||
test_name=test_name, | ||
output_file=output_file, | ||
output_format="openpmd", | ||
) | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We want to change the way we handle single-precision tests anyways (see #5205), so if this test is not a single-precision test, I think we can remove this function and simply call evaluate_checksum
below, consistently with the other analysis scripts in our test suite (since #5297):
def analyze_openpmd_regression(output_file): | |
# Run checksum regression test | |
if re.search("single_precision", output_file): | |
evaluate_checksum( | |
test_name=test_name, | |
output_file=output_file, | |
output_format="openpmd", | |
rtol=2e-6, | |
) | |
else: | |
evaluate_checksum( | |
test_name=test_name, | |
output_file=output_file, | |
output_format="openpmd", | |
) |
test_name = os.path.split(os.getcwd())[1] | ||
inst_output_file = sys.argv[1] | ||
|
||
# Regression checksum test | ||
# NOTE: works only in the example directory due to relative path import | ||
analyze_openpmd_regression(inst_output_file) | ||
|
||
# TODO: implement intervals parser for PICMI that allows more complex output periods | ||
if "picmi" not in test_name: | ||
# Functionality test for TimeAveragedDiagnostics | ||
compare_time_avg_with_instantaneous_diags( | ||
dir_inst=sys.argv[1], | ||
dir_avg="diags/diagTimeAvg/", | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given the comment above, I would rewrite it like this:
test_name = os.path.split(os.getcwd())[1] | |
inst_output_file = sys.argv[1] | |
# Regression checksum test | |
# NOTE: works only in the example directory due to relative path import | |
analyze_openpmd_regression(inst_output_file) | |
# TODO: implement intervals parser for PICMI that allows more complex output periods | |
if "picmi" not in test_name: | |
# Functionality test for TimeAveragedDiagnostics | |
compare_time_avg_with_instantaneous_diags( | |
dir_inst=sys.argv[1], | |
dir_avg="diags/diagTimeAvg/", | |
) | |
# NOTE: works only in the example directory due to relative path import | |
# compare checksums | |
evaluate_checksum( | |
test_name=os.path.split(os.getcwd())[1], | |
output_file=sys.argv[1], | |
) | |
# TODO: implement intervals parser for PICMI that allows more complex output periods | |
test_name = os.path.split(os.getcwd())[1] | |
if "picmi" not in test_name: | |
# Functionality test for TimeAveragedDiagnostics | |
compare_time_avg_with_instantaneous_diags( | |
dir_inst=sys.argv[1], | |
dir_avg="diags/diagTimeAvg/", | |
) |
If possible, I would leave the evaluate_checksum
call as in this suggestion (instead of passing, for example, test_name=test_name
with a test_name
variable defined before) because it matches the format of this "unit code block" across the test suite and will make it easier to implement some upcoming changes in a more automatic way.
This PR adds time-averaged field diagnostics to the WarpX output
Currently, compiling for GPU creates a linking error. We would like to see the CI output to possibly find the issue.
Hence, we do not flag this as WIP right now.
This PR is based on work performed during the 2024 WarpX Refactoring Hackathon and was created together with @RevathiJambunathan. 🙂
Successfully merging this pull request may close #5165.