[WIP] Deduplicate Moving Window Logic #6400
Conversation
After restarts, all diagnostics lo/hi were not properly restored. This causes new checkpoints and diagnostics to become corrupted, as the wrong spatial data gets filtered. This fixes it.
The math of a variable-window and variable-start/stop moving window is duplicated partly over the code base. This caused bugs. Refactoring to avoid bugs in the future.
ax3l
added
cleaning
Cell-aligned shifts are used in a few moving window places and hard-to-read code.
| const amrex::Real total_shift = WarpX::CalculateMovingWindowShift(0, last_replayed_step, warpx.getdt(0)); | ||
|
|
||
| if (total_shift > 0.0_rt) { | ||
| // Use the unified helper function to calculate physical shift aligned to cell boundaries | ||
| const amrex::Real physical_shift = WarpX::CalculateMovingWindowAlignedShift( | ||
| total_shift, warpx.Geom(0).CellSize(moving_dir)); |
There was a problem hiding this comment.
While we refactor this logic... Does restart (in development) even work with m_galilean_shift being active?
I would like to share even more logic that resides in FullDiagnostics::MovingWindowAndGalileanDomainShift here...
| if (WarpX::do_moving_window) { | ||
| // current moving_window location. Account for start_moving_window_step and | ||
| // end_moving_window_step to only shift based on steps where the window was active. | ||
| if (WarpX::do_moving_window && warpx.getistep(0) > 0) { |
There was a problem hiding this comment.
Why was warpx.getistep(0) > 0 added here?
Do we expect negative step number in WarpX?
There was a problem hiding this comment.
Added for only catching restarts, this PR is still wip as an idea to refactor. Forgot the title
| m_hi[moving_dir] += shift_num_base * warpx.Geom(0).CellSize(moving_dir); | ||
| const int current_step = warpx.getistep(0); | ||
|
|
||
| // Note: current_step is the checkpoint step, but the replay loop goes from 0 to current_step - 1 |
There was a problem hiding this comment.
What does "replay loop" mean here? Do you mean "restart"?
| const amrex::Real total_shift = WarpX::CalculateMovingWindowShift(0, last_replayed_step, warpx.getdt(0)); | ||
|
|
||
| if (total_shift > 0.0_rt) { | ||
| // Use the unified helper function to calculate physical shift aligned to cell boundaries |
There was a problem hiding this comment.
| // Use the unified helper function to calculate physical shift aligned to cell boundaries | |
| // Calculate physical shift aligned to cell boundaries |
| const amrex::Real* geom_dx = m_geom_output[0][0].CellSize(); | ||
| const auto num_shift_base = static_cast<int>((moving_window_x - cur_lo[moving_dir]) | ||
| / geom_dx[moving_dir]); | ||
| // Use the unified helper function to calculate physical shift aligned to cell boundaries |
There was a problem hiding this comment.
| // Use the unified helper function to calculate physical shift aligned to cell boundaries | |
| // Calculate physical shift aligned to cell boundaries |
| amrex::Real | ||
| WarpX::CalculateMovingWindowShiftPerStep (amrex::Real dt) | ||
| { | ||
| // Calculate the shift per step (same formula as used in MoveWindow) |
There was a problem hiding this comment.
| // Calculate the shift per step (same formula as used in MoveWindow) | |
| // Calculate the shift per step |
The formula does not exist anymore in MoveWindow, so this comment is confusing.
(Instead we now call CalculateMovingWindowShiftPerStep in MoveWindow ; but I don't think comments should point out where the function is called - or if they should, then we also need to say that this function is called in MovingWindowAndGalileanDomainShift
| /** Calculate the shift per step for the moving window | ||
| * | ||
| * This function calculates how much the moving window shifts in one timestep. | ||
| * This is the same calculation used in MoveWindow. |
There was a problem hiding this comment.
This function is used in MoveWindow, so it is confusing to say that this is the "same" calculation used in MoveWindow.
ax3l
changed the title
2 participants
The math of a variable-window and variable-start/stop moving window is duplicated partly over the code base. This caused bugs. Refactoring to avoid bugs in the future.