Add Time-Averaged Field Diagnostics #5285

n01r · 2024-09-18T01:23:52Z

This PR adds time-averaged field diagnostics to the WarpX output

code
docs
tests
example
meta-data $\Longrightarrow$ Follow-up PR
make compatible with adaptive time stepping $\Longrightarrow$ Follow-up PR

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.

archermarx · 2024-09-18T13:05:56Z

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.

n01r · 2024-09-18T16:33:15Z

Thanks, @archermarx! I wasn't aware of that yet. 🙂 We can do a follow-up and catch if dt_update_interval is set. In that case, we could create a cumulative sum weighted with the time step length and also add up the total time as we go. During the flush, we then divide by the total time.

archermarx · 2024-09-18T17:45:31Z

Thanks, @archermarx! I wasn't aware of that yet. 🙂 We can do a follow-up and catch if dt_update_interval is set. In that case, we could create a cumulative sum weighted with the time step length and also add up the total time as we go. During the flush, we then divide by the total time.

Yep, my thought exactly. The adaptive timestepping PR was just merged, too.

Source/Diagnostics/MultiDiagnostics.cpp

Source/Diagnostics/Diagnostics.cpp

Source/Diagnostics/FullDiagnostics.cpp

EZoni · 2024-09-19T00:16:53Z

I think #5296 should fix the clang-tidy errors that remain, which don't seem related to this PR.

ax3l · 2024-09-23T23:09:43Z

We can do a follow-up and catch if dt_update_interval is set
...
Yep, my thought exactly. The adaptive timestepping PR was just merged, too.

Thanks for the catch! Please add a runtime assert in this PR now.

n01r · 2024-09-24T19:13:41Z

Added commit that will cause the sim to abort when dt_update_interval is set.

The last thing for this PR that remains from our list is the CI tests.

@ax3l, should we also use the recently merged MultiFabRegister?

n01r · 2024-10-02T21:22:19Z

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:
inputs_test_2d_laser_ion_acc.txt

n01r · 2024-10-02T21:53:34Z

And comparing instantaneous output with averaged output for, e.g. Ez shows the much clearer structure one can see with this diagnostic.

EZoni · 2024-10-16T19:53:20Z

@n01r @RevathiJambunathan
After resolving the merge conflicts, do you think the feature added in this PR is complete and ready to go, pending review?

n01r · 2024-10-16T20:16:21Z

@EZoni, the CI tests are still missing. I was thinking of adding these today since Perlmutter is under maintenance anyway.

n01r · 2024-10-18T21:59:57Z

Examples/Physics_applications/laser_ion/CMakeLists.txt

@@ -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


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

Examples/Physics_applications/laser_ion/CMakeLists.txt

- 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

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.

for more information, see https://pre-commit.ci

n01r · 2024-10-18T23:32:05Z

@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.

for more information, see https://pre-commit.ci

Examples/Physics_applications/laser_ion/analysis_test_laser_ion.py

Modify checksum evaluation call to follow example from PR ECP-WarpX#5297. Co-authored-by: Edoardo Zoni <[email protected]>

for more information, see https://pre-commit.ci

EZoni · 2024-10-22T20:50:01Z

@n01r
If it's okay with you, I will push a fix for the CI tests (bug introduced by the changes I had suggested in my last review), rebase on the latest development, and then proceed with a final review.

n01r · 2024-10-22T20:54:36Z

Sounds good to me, @EZoni :)

…fields`

EZoni · 2024-10-22T21:16:31Z

@n01r

I guess this (in the PR description) isn't a problem anymore, right?

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.

EZoni

Thanks for this PR, looks great to me, and it's being thoroughly tested with your simulations. I left only a few minor comments.

Docs/source/usage/parameters.rst

Examples/Physics_applications/laser_ion/analysis_test_laser_ion.py

EZoni · 2024-10-22T21:41:42Z

Source/Diagnostics/Diagnostics.H

@@ -266,6 +270,12 @@ protected:
     *  The second vector is loops over the total number of levels.
     */
    amrex::Vector< amrex::Vector< amrex::MultiFab > >  m_mf_output;
+    /** summation multifab, where all fields (computed, cell-centered, and stacked)
+     *  are summed for every step in a time averaging period.
+     *  The first vector is for total number of snapshots. (=1 for FullDiagnostics)


Just checking, =1 in this comment means something like "only one element"? If so (that is, if 1 does not refer to the index to be used to access that vector layer), a more verbose description might be clearer.

Unsure. I think @RevathiJambunathan may have added/suggested this comment.

Actually, that was copy-pasted and then modified from the comment of the m_mf_output above.
We could consider making this in all the places where it appears in this file.
But a separate PR would probably be better for this.

Source/Diagnostics/Diagnostics.cpp

n01r · 2024-10-22T22:26:20Z

@n01r

I guess this (in the PR description) isn't a problem anymore, right?

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.

Oh, yea. That is not a thing anymore.
The CI also does not complain. I could check if locally I can compile for GPU in 3D because I think this was where I had some problems. But that may be my setup.

Co-authored-by: Edoardo Zoni <[email protected]>

Updating the relative tolerance to 1e-12 to keep it strict because the test compares two diagnostics that come from the same run on the same machine, just that one averaging happens in-situ while the other is done in post-processing. Co-authored-by: Edoardo Zoni <[email protected]>

EZoni · 2024-10-24T18:24:32Z

@n01r

Thanks! If possible, I would suggest that we track the work planned for follow-up PRs in an issue, just so we don't forget.

n01r · 2024-10-24T18:27:17Z

Agreed, @EZoni. Would you recommend continuing to use #5165 or opening new ones for individual tasks?

EZoni · 2024-10-24T18:30:23Z

Thanks, as you prefer. I think #5165 will be closed automatically because this PR is linked. You can either reopen that one after the PR is merged or open a new issue for follow-up work, as you prefer.

n01r added hackathon Let's address this topic during the GPU hackathon component: diagnostics all types of outputs labels Sep 18, 2024

EZoni reviewed Sep 18, 2024

View reviewed changes

Source/Diagnostics/MultiDiagnostics.cpp Show resolved Hide resolved

EZoni reviewed Sep 18, 2024

View reviewed changes

Source/Diagnostics/Diagnostics.cpp Outdated Show resolved Hide resolved

EZoni reviewed Sep 18, 2024

View reviewed changes

Source/Diagnostics/FullDiagnostics.cpp Outdated Show resolved Hide resolved

EZoni mentioned this pull request Sep 19, 2024

Fix clang-tidy errors in development branch #5296

Merged

n01r force-pushed the time_averaged_fields branch from af628fc to 822034d Compare September 20, 2024 22:40

ax3l self-assigned this Sep 23, 2024

n01r force-pushed the time_averaged_fields branch from 0f8c5e0 to 063f151 Compare September 24, 2024 19:08

RemiLehe assigned EZoni and unassigned ax3l Oct 16, 2024

n01r commented Oct 18, 2024

View reviewed changes

Examples/Physics_applications/laser_ion/CMakeLists.txt Outdated Show resolved Hide resolved

RevathiJambunathan and others added 7 commits October 18, 2024 15:10

skeleton sprinkled around

8dbde7d

Add time step interval logic

2707593

adding diag type to constructor and moving

a1b21d2

Add summation of MultiFabs

0aee250

Set time averaging options in ParmParse

1fef795

Continue averaging period logic

17d7620

- Implement warnings and errors for certain inputs concerning averaging periods

Add docs and scope code in if envs

c4b1775

- added first documentation on time averaged diags - put more operations on summation multifabs into if-environments

Added automated test based on LPI example

171ce3c

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.

n01r force-pushed the time_averaged_fields branch from bf71c29 to 171ce3c Compare October 18, 2024 22:31

[pre-commit.ci] auto fixes from pre-commit.com hooks

13cd6e4

for more information, see https://pre-commit.ci

Unify CI test for regression and TimeAvgDiag

22bc9a9

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.

n01r force-pushed the time_averaged_fields branch from 5979cc4 to 22bc9a9 Compare October 18, 2024 23:55

[pre-commit.ci] auto fixes from pre-commit.com hooks

0a46605

for more information, see https://pre-commit.ci

EZoni reviewed Oct 19, 2024

View reviewed changes

Examples/Physics_applications/laser_ion/analysis_test_laser_ion.py Outdated Show resolved Hide resolved

Examples/Physics_applications/laser_ion/analysis_test_laser_ion.py Outdated Show resolved Hide resolved

n01r and others added 2 commits October 21, 2024 10:59

Apply suggestions from code review

7c629d8

Modify checksum evaluation call to follow example from PR ECP-WarpX#5297. Co-authored-by: Edoardo Zoni <[email protected]>

[pre-commit.ci] auto fixes from pre-commit.com hooks

8a26f22

for more information, see https://pre-commit.ci

Fix CI tests (missing output_format argument)

83ae4b9

Merge ECP-WarpX:development into `RevathiJambunathan:time_averaged_…

6ac6212

…fields`

EZoni self-requested a review October 22, 2024 21:14

EZoni reviewed Oct 22, 2024

View reviewed changes

n01r and others added 2 commits October 22, 2024 15:32

Apply suggestions from code review

8b10e46

Co-authored-by: Edoardo Zoni <[email protected]>

Update comment in Diagnostics.cpp

b9fc39e

Co-authored-by: Edoardo Zoni <[email protected]>

EZoni self-requested a review October 23, 2024 00:12

EZoni approved these changes Oct 24, 2024

View reviewed changes

EZoni enabled auto-merge (squash) October 24, 2024 18:23

This was referenced Oct 24, 2024

Time-Averaged Field Diagnostics: Add Meta-Data #5410

Open

Time-Averaged Field Diagnostics: Adaptive Time-Stepping Compatibility #5411

Open

EZoni merged commit a25faff into ECP-WarpX:development Oct 24, 2024
37 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Time-Averaged Field Diagnostics #5285

Add Time-Averaged Field Diagnostics #5285

n01r commented Sep 18, 2024 •

edited

Loading

archermarx commented Sep 18, 2024

n01r commented Sep 18, 2024

archermarx commented Sep 18, 2024

EZoni commented Sep 19, 2024

ax3l commented Sep 23, 2024

n01r commented Sep 24, 2024

n01r commented Oct 2, 2024 •

edited

Loading

n01r commented Oct 2, 2024

EZoni commented Oct 16, 2024

n01r commented Oct 16, 2024

n01r Oct 18, 2024

n01r commented Oct 18, 2024

EZoni commented Oct 22, 2024

n01r commented Oct 22, 2024

EZoni commented Oct 22, 2024

EZoni left a comment

EZoni Oct 22, 2024

n01r Oct 22, 2024

n01r Oct 24, 2024

n01r commented Oct 22, 2024

EZoni commented Oct 24, 2024

n01r commented Oct 24, 2024

EZoni commented Oct 24, 2024

Add Time-Averaged Field Diagnostics #5285

Add Time-Averaged Field Diagnostics #5285

Conversation

n01r commented Sep 18, 2024 • edited Loading

archermarx commented Sep 18, 2024

n01r commented Sep 18, 2024

archermarx commented Sep 18, 2024

EZoni commented Sep 19, 2024

ax3l commented Sep 23, 2024

n01r commented Sep 24, 2024

n01r commented Oct 2, 2024 • edited Loading

n01r commented Oct 2, 2024

EZoni commented Oct 16, 2024

n01r commented Oct 16, 2024

n01r Oct 18, 2024

Choose a reason for hiding this comment

n01r commented Oct 18, 2024

EZoni commented Oct 22, 2024

n01r commented Oct 22, 2024

EZoni commented Oct 22, 2024

EZoni left a comment

Choose a reason for hiding this comment

EZoni Oct 22, 2024

Choose a reason for hiding this comment

n01r Oct 22, 2024

Choose a reason for hiding this comment

n01r Oct 24, 2024

Choose a reason for hiding this comment

n01r commented Oct 22, 2024

EZoni commented Oct 24, 2024

n01r commented Oct 24, 2024

EZoni commented Oct 24, 2024

n01r commented Sep 18, 2024 •

edited

Loading

n01r commented Oct 2, 2024 •

edited

Loading