-
Notifications
You must be signed in to change notification settings - Fork 121
Bug fix for CFL adaptive time stepping #1027
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
PR Reviewer Guide 🔍Here are some key observations to aid the review process:
|
| vcfl_dt = cfl_target*(min(dx(j), dy(k), fltr_dtheta)**2._wp) & | ||
| /minval(1/(rho*Re_l)) | ||
| /maxval(1/(rho*Re_l)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggestion: Prevent a potential division-by-zero error by adding a small epsilon value to the denominator rho*Re_l inside the maxval function call. [possible issue, importance: 8]
| vcfl_dt = cfl_target*(min(dx(j), dy(k), fltr_dtheta)**2._wp) & | |
| /minval(1/(rho*Re_l)) | |
| /maxval(1/(rho*Re_l)) | |
| vcfl_dt = cfl_target*(min(dx(j), dy(k), fltr_dtheta)**2._wp) & | |
| /maxval(1/(rho*Re_l + epsilon(0.0_wp))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR corrects the viscous CFL time-step calculation for 1D and 3D cases by changing minval to maxval when computing the kinematic viscosity term, ensuring consistency with the 2D case and the inverse stability calculation in s_compute_stability_from_dt.
- Fixes incorrect use of
minvaltomaxvalfor viscous CFL calculations - Ensures consistent physics across all dimensionalities (1D, 2D, 3D)
- Aligns with the inverse calculation in
s_compute_stability_from_dt
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1027 +/- ##
=======================================
Coverage 46.02% 46.02%
=======================================
Files 67 67
Lines 13437 13437
Branches 1550 1550
=======================================
Hits 6185 6185
Misses 6362 6362
Partials 890 890 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
User description
Description
Bug fix for CFL adaptive time stepping. The denominators should be
maxvalnotminvalin order to capture the worst case. Thanks to Mike VandenBoom for pointing this error out to me.Type of change
Please delete options that are not relevant.
Scope
PR Type
Bug fix
Description
Fix CFL adaptive time stepping denominator calculation
Change
minvaltomaxvalfor worst-case scenarioAffects 3D, 2D, and 1D viscous flow calculations
Diagram Walkthrough
File Walkthrough
m_sim_helpers.fpp
Replace minval with maxval in viscous CFL calculationssrc/simulation/m_sim_helpers.fpp
minval(1/(rho*Re_l))tomaxval(1/(rho*Re_l))in 3D viscouscase
minval(1/(rho*Re_l))tomaxval(1/(rho*Re_l))in 1D viscouscase