Refactor time_stepper #1012
Refactor time_stepper #1012
Conversation
PR Reviewer Guide 🔍Here are some key observations to aid the review process:
|
![qodo-merge-pro[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
| if (bodyForces) call s_apply_bodyforces(q_cons_ts(dest)%vf, q_prim_vf, rhs_vf, 2._wp*dt/3._wp) | ||
|
|
||
| if (grid_geometry == 3) call s_apply_fourier_filter(q_cons_ts(dest)%vf) | ||
| if (bodyForces) call s_apply_bodyforces(q_cons_ts(1)%vf, q_prim_vf, rhs_vf, rk_coef(s, 3)*dt) |
There was a problem hiding this comment.
Suggestion: The time step argument for s_apply_bodyforces has changed for the 2nd order scheme due to the refactoring. Use a select case block to apply the correct time step coefficients for each scheme and stage to maintain the original numerical behavior. [possible issue, importance: 8]
| if (bodyForces) call s_apply_bodyforces(q_cons_ts(1)%vf, q_prim_vf, rhs_vf, rk_coef(s, 3)*dt) | |
| if (bodyForces) then | |
| real(wp) :: bf_dt | |
| select case (time_stepper) | |
| case (1) ! 1st order | |
| bf_dt = dt | |
| case (2) ! 2nd order | |
| if (s == 1) then | |
| bf_dt = dt | |
| else | |
| bf_dt = 0.5_wp * dt | |
| end if | |
| case (3) ! 3rd order | |
| if (s == 1) then | |
| bf_dt = dt | |
| else if (s == 2) then | |
| bf_dt = 0.25_wp * dt | |
| else | |
| bf_dt = (2._wp/3._wp) * dt | |
| end if | |
| end select | |
| call s_apply_bodyforces(q_cons_ts(1)%vf, q_prim_vf, rhs_vf, bf_dt) | |
| end if |
There was a problem hiding this comment.
@hyeoksu-lee did you introduce a body forces bug? can't quite tell without looking closer
There was a problem hiding this comment.
Ask❓
@hyeoksu-lee did you introduce a body forces bug? can't quite tell without looking closer
Answer:
Likely yes: the refactor changes the time increment passed to s_apply_bodyforces in multi-stage schemes, altering numerical behavior.
What changed
- Previously, each TVD-RK routine applied body forces with scheme-specific dt weights:
- 1st order: dt
- 2nd order:
- Stage 1: dt
- Stage 2: 2/3 * dt (see call s_apply_bodyforces(..., 2._wp*dt/3._wp))
- 3rd order:
- Stage 1: dt
- Stage 2: dt/4
- Stage 3: 2/3 * dt
- After refactor, s_tvd_rk unconditionally calls:
- call s_apply_bodyforces(q_cons_ts(1)%vf, q_prim_vf, rhs_vf, rk_coef(s, 3)*dt)
- The rk_coef(s,3) you set equals:
- 1st order: [1]
- 2nd order: [1, 1/2]
- 3rd order: [1, 1/4, 2/3]
- Mismatch:
- 2nd order stage 2: old was 2/3dt, new is 1/2dt
- 3rd order matches the old weights (dt, dt/4, 2/3 dt)
- 1st order matches.
Impact
- For the 2nd order TVD-RK, body force contribution is reduced at stage 2 (0.5 vs 0.666... factor), which can alter accuracy and possibly stability for problems where body forces are significant.
Suggested fix
- Don’t tie body force dt to rk_coef(s,3). Restore scheme-specific weights for body forces:
- 1st order: dt
- 2nd order: stage 1 dt, stage 2 2/3*dt
- 3rd order: stage 1 dt, stage 2 dt/4, stage 3 2/3*dt
- Example patch inline with your loop:
- if (bodyForces) then
- select case (time_stepper)
- case (1); bf_dt = dt
- case (2); bf_dt = merge(dt, 2._wp*dt/3._wp, s == 1)
- case (3); bf_dt = merge(dt, merge(0.25_wpdt, 2._wpdt/3._wp, s == 3), s == 1)
- end select
- call s_apply_bodyforces(q_cons_ts(1)%vf, q_prim_vf, rhs_vf, bf_dt)
- select case (time_stepper)
- end if
- if (bodyForces) then
Bottom line
- Yes, there is a regression for time_stepper == 2: stage-2 body force dt should be 2/3dt, not 1/2dt.
There was a problem hiding this comment.
@sbryngelson Ah yes one coefficient I added is wrong. let me fix it.
By the way, why does the second-order Runge-Kutta method have a body force dt that is inconsistent with the time advancement of other variables? Who can I ask for more details?
There was a problem hiding this comment.
@wilfonba can comment. could be a bug, not sure. we don't often use rk2.
There was a problem hiding this comment.
I guess it could be a bug introduced by copying and pasting it from RK3... what s_apply_bodyforces does is basically q_cons_vf = q_cons_vf + dt * rhs_vf, so body_forces should advance the same time step I believe.
There was a problem hiding this comment.
Question:
I guess it could be a bug introduced by copying and pasting it from RK3... what s_apply_bodyforces does is basically q_cons_vf = q_cons_vf + dt * rhs_vf, so body_forces should advance the same time step I believe.
Answer:
To reduce future false positives and steer Qodo Merge suggestions toward your project’s RK time-stepping rules, add targeted extra instructions for the improve/review tools. For example, encode your scheme-specific body-force dt policy so the bot won’t propose conflicting coefficients:
[pr_code_suggestions]
extra_instructions = """\
Time-integration policy (do not change unless explicitly requested):
- s_apply_bodyforces advances q_cons_vf via q += dt*rhs over the exact stage dt used for other state updates.
- For TVD-RK:
* 1st order: dt
* 2nd order: stage1 dt, stage2 2/3*dt (or project standard)
* 3rd order: stage1 dt, stage2 1/4*dt, stage3 2/3*dt
Reject suggestions that tie body-force dt to generic rk_coef indices unless they reproduce these exact weights.
"""
[pr_reviewer]
extra_instructions = """\
Validate RK stage coefficients against project-standard time-stepping:
- Body forces must use the same effective stage dt as conserved variables.
- Flag any change that alters numerical behavior by mixing RK weight arrays with body-force dt.
"""If you keep best practices centrally, also capture this as a rule in best_practices.md so the guidance is consistently applied across PRs.
Relevant Sources:
- https://qodo-merge-docs.qodo.ai//tools/improve#extra-instructions-and-best-practices
- https://qodo-merge-docs.qodo.ai//tools/review#configuration-options
- https://qodo-merge-docs.qodo.ai//usage-guide/configuration_options#the-different-tools-and-sub-tools-used-by-qodo-merge-are-adjustable-via-a-git-configuration-file
|
I love a good red line PR! Will review soon |
|
Some of test suite failed (with the error close to tolerance, though), so set this PR as draft for now |
|
small errors when refactoring the time steppers was also seen by @danieljvickers... so long as i'm convinced the code is right we might need a new goldenfile that satiates all the time steppers (or figure out the offending case and set looser tolerance). |
PR Reviewer Guide 🔍Here are some key observations to aid the review process:
|
|
Oh I see.. I put it back to ready for review. Please review the change and let me know if you have any question or concerns! |
There was a problem hiding this comment.
High-level Suggestion
To improve readability and maintainability, define the Runge-Kutta coefficients as named constants or parameters instead of hardcoding them in a large if-else block. [High-level, importance: 6]
Solution Walkthrough:
Before:
subroutine s_initialize_time_steppers_module
...
if (any(time_stepper == (/1, 2, 3/))) then
@:ALLOCATE (rk_coef(time_stepper, 3))
if (time_stepper == 1) then
rk_coef(1, 1) = 1._wp
rk_coef(1, 2) = 0._wp
rk_coef(1, 3) = 1._wp
else if (time_stepper == 2) then
rk_coef(1, 1) = 1._wp; rk_coef(1, 2) = 0._wp; rk_coef(1, 3) = 1._wp
rk_coef(2, 1) = 0.5_wp; rk_coef(2, 2) = 0.5_wp; rk_coef(2, 3) = 0.5_wp
else if (time_stepper == 3) then
...
end if
end if
end subroutine
After:
module m_time_steppers
...
real(wp), parameter :: RK1_COEFS(1,3) = reshape((/ 1.0_wp, 0.0_wp, 1.0_wp /), (/1,3/))
real(wp), parameter :: RK2_COEFS(2,3) = reshape((/ 1.0_wp, 0.0_wp, 1.0_wp, &
0.5_wp, 0.5_wp, 0.5_wp /), (/2,3/))
...
contains
subroutine s_initialize_time_steppers_module
...
if (time_stepper == 1) then
rk_coef = RK1_COEFS
else if (time_stepper == 2) then
rk_coef = RK2_COEFS
...
end if
end subroutine
Co-authored-by: qodo-merge-pro[bot] <151058649+qodo-merge-pro[bot]@users.noreply.github.com>
Co-authored-by: qodo-merge-pro[bot] <151058649+qodo-merge-pro[bot]@users.noreply.github.com>
|
Oh yeah, I tried to do something similar a few months ago, but kept getting weird compiler-dependent bugs that I couldn't reproduce locally. I like this PR. Looks like it is reducing a lot of the redundant code in the time_stepper. Seems odd that you are failing so many tests by so little. I would be interested to also see how far off of the golden files you are for the tests that pass, and see if they are similar. |
|
@hyeoksu-lee status on this? was working i guess, but now everything is failing? |
|
@sbryngelson All failing CPU tests are due to error of the order of 1e-10, which is tolerance level. I am looking into the code to see if there is any systematic issue. And I am waiting for @wilfonba to review body force input for RK2. In the meantime, I need to fix GPU bug, which seems not a simple tolerance-type issue. |
User description
Description
This PR refactors time_stepper routines by integrating s_1st_order_tvd_rk, s_2nd_order_tvd_rk, s_3rd_order_tvd_rk, s_strang_splitting into a single subroutine s_tvd_rk.
Type of change
Scope
If you cannot check the above box, please split your PR into multiple PRs that each have a common goal.
How Has This Been Tested?
Please describe the tests that you ran to verify your changes.
Provide instructions so we can reproduce.
Please also list any relevant details for your test configuration
Test Configuration:
Checklist
docs/)examples/that demonstrate my new feature performing as expected.They run to completion and demonstrate "interesting physics"
./mfc.sh formatbefore committing my codeIf your code changes any code source files (anything in
src/simulation)To make sure the code is performing as expected on GPU devices, I have:
nvtxranges so that they can be identified in profiles./mfc.sh run XXXX --gpu -t simulation --nsys, and have attached the output file (.nsys-rep) and plain text results to this PR./mfc.sh run XXXX --gpu -t simulation --rsys --hip-trace, and have attached the output file and plain text results to this PR.PR Type
Enhancement
Description
Consolidate four separate TVD RK time-stepping subroutines into single unified
s_tvd_rkReplace conditional calls with unified interface accepting
nstageparameterAdd Runge-Kutta coefficient matrix for different time-stepping orders
Integrate adaptive time-stepping for bubble dynamics into unified routine
Diagram Walkthrough
File Walkthrough
m_start_up.fpp
Simplify time-stepper invocation logicsrc/simulation/m_start_up.fpp
s_tvd_rkcalltime_stepperparameter to unified routinem_time_steppers.fpp
Consolidate TVD RK time-stepping implementationssrc/simulation/m_time_steppers.fpp
s_1st_order_tvd_rk,s_2nd_order_tvd_rk,s_3rd_order_tvd_rk,s_strang_splitting)s_tvd_rksubroutine with stage loop and RK coefficientmatrix