Remove buggy synchronization at end of steps #5820
Conversation
dpgrote
added
bug
|
@dpgrote Thanks for this PR! Would you be able to merge |
Source/Evolve/WarpXEvolve.cpp
Outdated
| *m_fields.get(FieldType::Bfield_aux, Direction{1}, lev), | ||
| *m_fields.get(FieldType::Bfield_aux, Direction{2}, lev) | ||
| ); | ||
| if (!is_synchronized) { |
There was a problem hiding this comment.
This may be one of those examples where making it clear that is_synchronized is a member variable of the WarpX class, through the prefix m_, would help not get lost while reading the code. For a minute I was trying to understand where is_synchronized is defined in this function. We could rename it according to the WarpX style rules in a separate PR.
|
Two big PRs are being merged as we speak:
Since those PRs introduce new CI tests, I would recommend that we merge |
|
I could use this fix after #3630 was merged. |
EZoni
added
the
bug: affects latest release
|
After the last merge of |
| @@ -351,12 +337,13 @@ WarpX::Evolve (int numsteps) | |||
| } | |||
|
|
|||
| bool const do_diagnostic = (multi_diags->DoComputeAndPack(step) || reduced_diags->DoDiags(step)); | |||
| bool const end_of_step_loop = (step == numsteps_max - 1) || (cur_time + dt[0] >= stop_time - 1.e-3*dt[0]); | |||
There was a problem hiding this comment.
Just a question to clarify, where does the 1.e-3 factor come from here?
There was a problem hiding this comment.
It's a somewhat arbitrary factor to allow for round off error in the time. If the cur_time + dt is almost but not quite equal to stop_time, then that's close enough.
| assert Ep_f < 0.7 * Ep_i # Check that potential energy changes significantly | ||
| assert abs((Ek_i + Ep_i) - (Ek_f + Ep_f)) < 0.003 * ( | ||
| assert abs((Ek_i + Ep_i) - (Ek_f + Ep_f)) < 0.0032 * ( |
There was a problem hiding this comment.
I don't know why the error is worse here than before. Note that this has warpx.synchronize_velocity_for_diagnostics turned on. With it off, the error is a factor of several worse.
Weekly update to latest AMReX, pyAMReX, PICSAR: ``` ./Tools/Release/updateAMReX.py ./Tools/Release/updatepyAMReX.py ./Tools/Release/updatePICSAR.py ``` WarpX release 25.05: ``` ./Tools/Release/newVersion.sh ``` To-do: - [x] Merge AMReX-Codes/pyamrex#442 - [x] Merge #5820 - [x] Mark as ready for review and run CI
This PR does cleanup, removing a potential bug that happens when the velocity is synchronized with position in the final diagnostic. The particle boundary conditions had not been applied so the field gather could be out of bounds. Further cleanup is done, renaming Synchronize to SynchronizeVelocityWithPosition, and having it check is_synchronized, since the synchronization may have already been done. Note that the factor `1e-3` in the code change ```cpp bool const end_of_step_loop = (step == numsteps_max - 1) || (cur_time + dt[0] >= stop_time - 1.e-3*dt[0]); ``` is a somewhat arbitrary factor to allow for round off error in the time. If the `cur_time + dt[0]` is almost but not quite equal to `stop_time`, then that's close enough. Comment copied here from BLAST-WarpX#5820 (comment) for better visibility.
Weekly update to latest AMReX, pyAMReX, PICSAR: ``` ./Tools/Release/updateAMReX.py ./Tools/Release/updatepyAMReX.py ./Tools/Release/updatePICSAR.py ``` WarpX release 25.05: ``` ./Tools/Release/newVersion.sh ``` To-do: - [x] Merge AMReX-Codes/pyamrex#442 - [x] Merge BLAST-WarpX#5820 - [x] Mark as ready for review and run CI
This PR does cleanup, removing a potential bug that happens when the velocity is synchronized with position in the final diagnostic. The particle boundary conditions had not been applied so the field gather could be out of bounds.
Further cleanup is done, renaming Synchronize to SynchronizeVelocityWithPosition, and having it check is_synchronized, since the synchronization may have already been done.
Note that the factor
1e-3in the code changeis a somewhat arbitrary factor to allow for round off error in the time. If the
cur_time + dt[0]is almost but not quite equal tostop_time, then that's close enough. Comment copied here from #5820 (comment) for better visibility.