Skip to content

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Bug fix for CFL adaptive time stepping #1027

wants to merge 1 commit into from

Conversation

@wilfonba
Copy link
Collaborator

@wilfonba wilfonba commented Nov 5, 2025 •
edited by qodo-merge-pro bot
Loading

User description

Description

Bug fix for CFL adaptive time stepping. The denominators should be maxval not minval in 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.

  • Bug fix (non-breaking change which fixes an issue)

Scope

  • This PR comprises a set of related changes with a common goal

PR Type

Bug fix

Description

  • Fix CFL adaptive time stepping denominator calculation

  • Change minval to maxval for worst-case scenario

  • Affects 3D, 2D, and 1D viscous flow calculations

Diagram Walkthrough

flowchart LR
  A["CFL Time Stepping<br/>Viscous Calculations"] -->|"Replace minval<br/>with maxval"| B["Worst-case<br/>Denominator"]
  B --> C["Corrected dt<br/>Calculation"]
Loading

File Walkthrough
Relevant files
Bug fix
m_sim_helpers.fpp
Replace minval with maxval in viscous CFL calculations     

src/simulation/m_sim_helpers.fpp

  • Changed minval(1/(rho*Re_l)) to maxval(1/(rho*Re_l)) in 3D viscous
    case
  • Changed minval(1/(rho*Re_l)) to maxval(1/(rho*Re_l)) in 1D viscous
    case
  • Ensures worst-case scenario is captured for CFL stability criterion
 +3/-3     

Copilot AI review requested due to automatic review settings November 5, 2025 17:04
@wilfonba wilfonba requested a review from a team as a code owner November 5, 2025 17:04
@qodo-merge-pro
Copy link
Contributor

qodo-merge-pro bot commented Nov 5, 2025

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
🧪 No relevant tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

Possible Issue

Mixed use of maxval with differently arranged expressions could lead to inconsistent behavior across dimensions; verify that 2D path using maxval((1/Re_l)/rho) is equivalent to maxval(1/(rho*Re_l)) and that all arrays share the same masking/shape context.

 
    vcfl_dt = cfl_target*(min(dx(j), dy(k))**2._wp)/maxval((1/Re_l)/rho)
else
    !1D
    vcfl_dt = cfl_target*(dx(j)**2._wp)/maxval(1/(rho*Re_l))
end if
Numerical Stability

Division by maxval(1/(rhoRe_l)) can blow up if rhoRe_l approaches zero; ensure lower bounds or safeguards (e.g., eps floor) are applied to avoid inf or NaN time steps.

 
        vcfl_dt = cfl_target*(min(dx(j), dy(k), fltr_dtheta)**2._wp) &
                  /maxval(1/(rho*Re_l))
    else
        vcfl_dt = cfl_target*(min(dx(j), dy(k), dz(l))**2._wp) &
                  /maxval(1/(rho*Re_l))
    end if
elseif (n > 0) then
    !2D
    vcfl_dt = cfl_target*(min(dx(j), dy(k))**2._wp)/maxval((1/Re_l)/rho)
else
    !1D
    vcfl_dt = cfl_target*(dx(j)**2._wp)/maxval(1/(rho*Re_l))
end if

qodo-merge-pro[bot]
qodo-merge-pro bot reviewed Nov 5, 2025
View reviewed changes
src/simulation/m_sim_helpers.fpp
Comment on lines 273 to +274
vcfl_dt = cfl_target*(min(dx(j), dy(k), fltr_dtheta)**2._wp) &
/minval(1/(rho*Re_l))
/maxval(1/(rho*Re_l))
Copy link
Contributor

@qodo-merge-pro qodo-merge-pro bot Nov 5, 2025

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]

Suggested change
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)))

Copilot AI reviewed Nov 5, 2025
View reviewed changes
Copy link
Contributor

Copilot AI left a 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 minval to maxval for viscous CFL calculations
  • Ensures consistent physics across all dimensionalities (1D, 2D, 3D)
  • Aligns with the inverse calculation in s_compute_stability_from_dt

@codecov
Copy link

codecov bot commented Nov 5, 2025

Codecov Report

❌ Patch coverage is 0% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 46.02%. Comparing base (bfd732c) to head (56396aa).
⚠️ Report is 2 commits behind head on master.

Files with missing lines Patch % Lines
src/simulation/m_sim_helpers.fpp 0.00% 1 Missing ⚠️
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.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@qodo-merge-pro qodo-merge-pro[bot] qodo-merge-pro[bot] left review comments

Copilot code review Copilot Copilot left review comments

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

Review effort 2/5

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@wilfonba