Synchronize velocity for diagnostics #1751

dpgrote · 2021-03-02T00:16:31Z

This PR allows the synchronization in time of the particle velocities and positions when generating diagnostics. Without this option, the particle velocities will lag behind the position by a half time step. This adds the boolean input parameter warpx.synchronize_velocity_for_diagnostics to turn on this option, defaulting to false.

There are several pieces to this PR:

Changes to MultiDiagnostic and MultiReducedDiags adding routines to check if any diagnostics will be done
Adds a call to PushP to just before the diagnostics are done (to get the updated fields from the electrostatic calculation)
Add the appropriate documentation

What Evolve does is if the synchronization is to be done, advance the velocity a half step just before the diagnostics and sets is_synchronized=true. Then at the start of the next step, if is_synchronized is true, push the velocities back a half step to be ready for the full leap frog advance.

Comments:

Is the documentation in the correct place in parameters.rst?
The reduced diagnostics could perhaps use the new DoDiags method instead of accessing m_intervals in its ComputeDiags.
This PR leaves the original PushP unchanged, even though it is possibly buggy. That PushP fetches the fields, but uses the particle positions before the particle boundary conditions have been applied, leading to a possible out of bounds reference. Also, that PushP may not be consistent with the backwards PushP since the fields may had changed. Comments are added to the code to note this potential problem. I avoided changing this since it breaks many CI tests.

RemiLehe · 2021-03-02T16:59:07Z

Source/Evolve/WarpXEvolve.cpp

+            (step == numsteps_max-1) ||
+            (synchronize_velocity_for_diagnostics &&
+                (multi_diags->DoComputeAndPack(step) ||
+                 reduced_diags->DoDiags(step)))) {


The above if condition looks like should not modify the default behavior of the code (which is probably what we want). However, it seems that a lot of automated tests are failing because of checkSum differences. @dpgrote Do you understand why?

Also, could you explain why this block of code was moved here?

Hi @RemiLehe - I moved the block of code to the end of the loop so that the state of the data when the half v push is done is the same as it will be at the start of the next step when the half push will be undone. Ideally, they should exactly cancel each other. This is particularly important when running with electrostatic where the push needs to be done after the new fields are computed. It seems that this block should be here anyway, since the field gather done in the PushP should be done after the particle boundary conditions are applied.

This would explain why some of the CI tests are failing, that something was changed between where this block was and where it is now. I'll look into it to understand what is happening.

RemiLehe · 2021-03-08T19:25:55Z

Thanks for answering the above question.
It looks like a few additional checksums need to be updated, but then, after that, the PR would be ready to be merged.

ax3l · 2021-03-29T22:21:39Z

@dpgrote just a little ping since there is a slight merge conflict in this PR that needs to be resolved now.

Note that the parameters.rst file moved now due to #1821

dpgrote · 2021-03-29T23:17:42Z

Thanks @ax3l . I need to get back to this one and finish it. There are some CI differences that I need to make sure I understand.

…velocity_for_diagnostics

RemiLehe · 2021-05-24T19:35:10Z

@dpgrote Should we change this to [WIP] while the CI issues are being investigated?

…velocity_for_diagnostics

…agnostic)

ax3l · 2023-02-10T22:05:03Z

Source/Evolve/WarpXEvolve.cpp

@@ -217,8 +217,16 @@ WarpX::Evolve (int numsteps)
            // B : guard cells are NOT up-to-date
        }

-        if (cur_time + dt[0] >= stop_time - 1.e-3*dt[0] || step == numsteps_max-1) {
+        if ((cur_time + dt[0] >= stop_time - 1.e-3*dt[0] || step == numsteps_max-1) &&


This is a bit long. Maybe a temporary bool could simplify the logic in the if?

Thanks for the comments! Do you have a suggestion of how to simplify this?

ax3l · 2023-02-10T22:05:22Z

Source/Evolve/WarpXEvolve.cpp

            // At the end of last step, push p by 0.5*dt to synchronize
+            // Note that this is potentially buggy since the PushP will do a field gather


Do we need a couple of input option asserts to avoid potentially buggy behavior?

The buggy behavior regarding particle bounds is not dependent on input options, but will always happen. For the other, Python with electrostatic, this should just be fixed by removing this block of code and using the one below instead. I can do this in a separate PR because it will break a number of CI tests.

EZoni · 2025-01-15T18:11:38Z

@dpgrote

Hi Dave, do you think this is a change that we would still like to merge? I think so, I see it was mentioned from another PR last year. If this is still relevant, do you think it is possible to merge development and fix the merge conflicts or would it be easier to just start from scratch with a new PR?

dpgrote · 2025-01-15T19:02:48Z

@dpgrote

Hi Dave, do you think this is a change that we would still like to merge? I think so, I see it was mentioned from another PR last year. If this is still relevant, do you think it is possible to merge development and fix the merge conflicts or would it be easier to just start from scratch with a new PR?

@EZoni I would still like to have this PR merged. I can merge in development and see how bad the conflicts are.

…diagnostics`

EZoni · 2025-01-29T01:54:32Z

@dpgrote

I took the liberty to merge development. Please feel free to have a look to double check that I did not miss anything while fixing the merge conflicts, whenever you have time. Let me know if the new code changes look correct to you and consistent with the original implementation.

…d]]`

dpgrote · 2025-01-29T23:16:42Z

@dpgrote

I took the liberty to merge development. Please feel free to have a look to double check that I did not miss anything while fixing the merge conflicts, whenever you have time. Let me know if the new code changes look correct to you and consistent with the original implementation.

Thanks. The changes look correct, following the same pattern.

EZoni · 2025-01-30T00:20:30Z

Thanks for checking, @dpgrote. Note that HIP is failing independently of this PR (due to a new ROCm release) and expected to resolve itself in a few days.

…diagnostics`

EZoni · 2025-02-05T19:57:39Z

@dpgrote

I saw that the HIP checks were finally fixed, but there was another minor merge conflict, so I merged development again and fixed the conflict. Hopefully this should be ready for review/merge now.

Docs/source/usage/parameters.rst

ax3l · 2025-02-11T00:46:44Z

Source/Evolve/WarpXEvolve.cpp

+            if ((cur_time + dt[0] >= stop_time - 1.e-3*dt[0] || step == numsteps_max-1) &&
+                !synchronize_velocity_for_diagnostics) {


Can you please add a helper function that returns true/false for if the last step query of the first part of this if and then simplify this if?

Also, you can reuse the logic for the if below.

makes the if look like

if (end_of_last_step && !synchronize_velocity_for_diagnostics) // existing if ... if (end_of_last_step && (other_stuff || synchronize_velocity_for_diagnostics)) // extra if below that was added

Is this better?

ax3l · 2025-02-11T00:49:40Z

Source/Evolve/WarpXEvolve.cpp

@@ -309,6 +317,15 @@ WarpX::Evolve (int numsteps)
            ExecutePythonCallback("afterEsolve");
        }

+        if (synchronize_velocity_for_diagnostics &&
+            (multi_diags->DoComputeAndPack(step) || reduced_diags->DoDiags(step) ||


Please see suggestion above.
Please assign the logic behind multi_diags->DoComputeAndPack(step) || reduced_diags->DoDiags(step) to a named boolean as well, because otherwise this if gets a bit involved to read.

Co-authored-by: Axel Huebl <[email protected]>

Examples/Tests/single_particle/analysis_synchronize_velocity.py

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

EZoni · 2025-02-14T01:26:10Z

Examples/Tests/single_particle/analysis_synchronize_velocity.py

+uz -= -e / m_e * Ez * dt / 2.0
+
+# Leap frog advance
+for i in range(5):


To fix CodeQL's issue:

Suggested change

for i in range(5):

for _ in range(5):

dpgrote added 3 commits March 1, 2021 15:23

Add routine to check if any diagnostics will be done

c06bf14

Added synchronize_velocity_for_diagnostics input parameter

5870994

Added documentation for synchronize_velocity_for_diagnostics

136b056

dpgrote added the component: diagnostics all types of outputs label Mar 2, 2021

RemiLehe self-requested a review March 2, 2021 01:20

RemiLehe self-assigned this Mar 2, 2021

RemiLehe reviewed Mar 2, 2021

View reviewed changes

RemiLehe approved these changes Mar 8, 2021

View reviewed changes

dpgrote added 3 commits April 1, 2021 16:45

Merge remote-tracking branch 'ECPwarpx/development' into synchronize_…

a3ae45d

…velocity_for_diagnostics

Merge remote-tracking branch 'ECPwarpx/development' into synchronize_…

6a76009

…velocity_for_diagnostics

Fixed pp_warpx for synchronize_velocity_for_diagnostics

c3e8f06

dpgrote changed the title ~~Synchronize velocity for diagnostics~~ [WIP]Synchronize velocity for diagnostics May 24, 2021

dpgrote added 13 commits June 11, 2021 09:51

Merge remote-tracking branch 'ECPwarpx/development' into synchronize_…

459f074

…velocity_for_diagnostics

Bug fix, check cur_time (not cur_time + dt)

07cf2f3

Merge remote-tracking branch 'ECPwarpx/development' into synchronize_…

0d2617a

…velocity_for_diagnostics

Merge remote-tracking branch 'ECPwarpx/development' into synchronize_…

3cf522f

…velocity_for_diagnostics

Merge remote-tracking branch 'ECPwarpx/development' into synchronize_…

c91b148

…velocity_for_diagnostics

Merge remote-tracking branch 'ECPwarpx/development' into synchronize_…

e17704b

…velocity_for_diagnostics

Merge remote-tracking branch 'ECPwarpx/development' into synchronize_…

a72aea1

…velocity_for_diagnostics

For synchronize, only call fill boundary with EM

89d5c21

Merge branch 'development' into synchronize_velocity_for_diagnostics

2d15c04

Updates to match development

2580217

Update comment

61a8f77

Fix the check for the final step (to match the check for the final di…

e8a8142

…agnostic)

The synchronization is only done when the flag is true

a3a823a

dpgrote force-pushed the synchronize_velocity_for_diagnostics branch from 0fe6cbe to a3a823a Compare January 18, 2023 18:41

Add warpx_synchronize_velocity flag to picmi interface

48e6f92

ax3l reviewed Feb 10, 2023

View reviewed changes

dpgrote added 3 commits March 17, 2023 16:07

Merge branch 'development' into synchronize_velocity_for_diagnostics

ef50556

Merge branch 'development' into synchronize_velocity_for_diagnostics

c4910b2

Merge branch 'development' into synchronize_velocity_for_diagnostics

383ec52

dpgrote mentioned this pull request Aug 16, 2024

Add call to HandleParticlesAtBoundaries in WarpXEvolve.cpp to prevent oob segfaults #5136

Closed

EZoni self-requested a review January 15, 2025 20:05

EZoni self-assigned this Jan 15, 2025

Merge ECP-WarpX:development into `dpgrote:synchronize_velocity_for_…

54ce2d6

…diagnostics`

EZoni added 2 commits January 29, 2025 11:17

Fix clang-tidy error: method DoDiags can be made const

876f6b2

Fix clang-tidy error: function DoDiags should be marked `[[nodiscar…

eba2e29

…d]]`

Merge ECP-WarpX:development into `dpgrote:synchronize_velocity_for_…

5353a51

…diagnostics`

EZoni requested a review from roelof-groenewald February 11, 2025 00:40

ax3l reviewed Feb 11, 2025

View reviewed changes

Docs/source/usage/parameters.rst Outdated Show resolved Hide resolved

ax3l reviewed Feb 11, 2025

View reviewed changes

dpgrote and others added 3 commits February 12, 2025 09:44

Fix double back quote in documentation

60df9dd

Co-authored-by: Axel Huebl <[email protected]>

Fix small error and clean up complicated if conditions

7cd5327

Add CI test

97b3b1a

github-advanced-security bot found potential problems Feb 12, 2025

View reviewed changes

Examples/Tests/single_particle/analysis_synchronize_velocity.py Fixed Show fixed Hide fixed

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

7656bb4

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

github-advanced-security bot found potential problems Feb 12, 2025

View reviewed changes

Fix const

35221d6

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Synchronize velocity for diagnostics #1751

Synchronize velocity for diagnostics #1751

dpgrote commented Mar 2, 2021 •

edited

Loading

RemiLehe Mar 2, 2021

RemiLehe Mar 2, 2021

dpgrote Mar 2, 2021

RemiLehe commented Mar 8, 2021

ax3l commented Mar 29, 2021

dpgrote commented Mar 29, 2021

RemiLehe commented May 24, 2021

ax3l Feb 10, 2023

dpgrote Feb 10, 2023

ax3l Feb 10, 2023

dpgrote Feb 10, 2023

EZoni commented Jan 15, 2025

dpgrote commented Jan 15, 2025

EZoni commented Jan 29, 2025

dpgrote commented Jan 29, 2025

EZoni commented Jan 30, 2025

EZoni commented Feb 5, 2025

ax3l Feb 11, 2025 •

edited

Loading

dpgrote Feb 12, 2025

ax3l Feb 11, 2025

dpgrote Feb 12, 2025

EZoni Feb 14, 2025

		// At the end of last step, push p by 0.5*dt to synchronize
		// Note that this is potentially buggy since the PushP will do a field gather

		if ((cur_time + dt[0] >= stop_time - 1.e-3*dt[0] \|\| step == numsteps_max-1) &&
		!synchronize_velocity_for_diagnostics) {

Synchronize velocity for diagnostics #1751

Are you sure you want to change the base?

Synchronize velocity for diagnostics #1751

Conversation

dpgrote commented Mar 2, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RemiLehe commented Mar 8, 2021

ax3l commented Mar 29, 2021

dpgrote commented Mar 29, 2021

RemiLehe commented May 24, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

EZoni commented Jan 15, 2025

dpgrote commented Jan 15, 2025

EZoni commented Jan 29, 2025

dpgrote commented Jan 29, 2025

EZoni commented Jan 30, 2025

EZoni commented Feb 5, 2025

ax3l Feb 11, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dpgrote commented Mar 2, 2021 •

edited

Loading

ax3l Feb 11, 2025 •

edited

Loading