Bug fix for CFL adaptive time stepping

[wilfonba]

User description

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

Note

Replace minval(1/(rho*Re_l)) with maxval(1/(rho*Re_l)) in viscous CFL timestep calculations to enforce the strongest constraint in 3D and 1D paths.

• Time stepping (CFL) in src/simulation/m_sim_helpers.fpp:
  • Viscous dt: Replace minval(1/(rho*Re_l)) with maxval(1/(rho*Re_l)) in s_compute_dt_from_cfl.
    • 3D: Updates both cylindrical (grid_geometry == 3) and Cartesian branches.
    • 1D: Applies the same correction.
  • Leaves 2D handling unchanged (already uses worst-case form).

^{Written by Cursor Bugbot for commit 2485e7e. Configure here.}

---

CodeAnt-AI Description

Base CFL viscous time step on worst-case viscosity term

What Changed

• Viscous CFL computation now divides by the largest 1/(rho*Re_l) factor so 3D and 1D flows hit the worst-case denominator instead of the minimum value
• Time-step selection now enforces the most restrictive viscous limit before comparing with the inviscid CFL criterion
• This guarantees each viscous update respects the strongest viscosity contribution across the grid

Impact

✅ Avoids CFL violations in viscous runs
✅ Ensures 3D/1D viscous steps stay within the strictest time limit
✅ Keeps simulation stability tied to worst-case viscosity

💡 Usage Guide

Checking Your Pull Request

Every time you make a pull request, our system automatically looks through it. We check for security issues, mistakes in how you're setting up your infrastructure, and common code problems. We do this to make sure your changes are solid and won't cause any trouble later.

Talking to CodeAnt AI

Got a question or need a hand with something in your pull request? You can easily get in touch with CodeAnt AI right here. Just type the following in a comment on your pull request, and replace "Your question here" with whatever you want to ask:

@codeant-ai ask: Your question here

This lets you have a chat with CodeAnt AI about your pull request, making it easier to understand and improve your code.

Example

@codeant-ai ask: Can you suggest a safer alternative to storing this secret?

Preserve Org Learnings with CodeAnt

You can record team preferences so CodeAnt AI applies them in future reviews. Reply directly to the specific CodeAnt AI suggestion (in the same thread) and replace "Your feedback here" with your input:

@codeant-ai: Your feedback here

This helps CodeAnt AI learn and adapt to your team's coding style and standards.

Example

@codeant-ai: Do not flag unused imports.

Retrigger review

Ask CodeAnt AI to review the PR again, by typing:

@codeant-ai: review

Check Your Repository Health

To analyze the health of your code repository, visit our dashboard at https://app.codeant.ai. This tool helps you identify potential issues and areas for improvement in your codebase, ensuring your repository maintains high standards of code health.

[qodo-code-review]

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-code-review]

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]

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]

Codecov Report

❌ Patch coverage is 0% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 44.36%. Comparing base (e25dbc4) to head (2485e7e).
⚠️ Report is 1 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   44.36%   44.36%           
=======================================
  Files          71       71           
  Lines       20590    20590           
  Branches     1994     1994           
=======================================
  Hits         9134     9134           
  Misses      10310    10310           
  Partials     1146     1146           

☔ 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.

[codeant-ai]

CodeAnt AI is reviewing your PR.

[coderabbitai]

Note

Other AI code review bot(s) detected

CodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review.

Walkthrough

This change modifies viscous CFL timestep calculations in a simulation helper module. Three instances of minval() function calls are replaced with maxval() in the timestep computation logic across different grid geometry branches (3D and 1D configurations), resulting in less restrictive viscous scaling constraints.

Changes

Cohort / File(s)	Summary
Viscous CFL Timestep Calculations src/simulation/m_sim_helpers.fpp	Replaced minval(1/(rho*Re_l)) with maxval(1/(rho*Re_l)) in three branches of s_compute_dt_from_cfl: 3D grid (grid_geometry == 3) with p > 0, 3D grid else branch with p > 0, and 1D grid viscous case. Changes shift dt scaling from minimum-value-based to maximum-value-based constraint logic.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

• Key consideration: The numerical impact of switching from minval to maxval on CFL stability and physical accuracy should be verified; confirm this change aligns with intended simulation behavior.
• Attention areas: All three modified locations handle similar logic but apply to different grid dimensions; ensure consistency in reasoning across 3D and 1D cases and verify test coverage for affected timestep computations.

Poem

🐰 A timestep dance of min and max,
We flip the script and relax,
From strictest bounds to roomy lanes,
The viscous flow now sustains,
CFL's new breath, less constrained!

Pre-merge checks and finishing touches

❌ Failed checks (1 inconclusive)

Check name	Status	Explanation	Resolution
Description check	❓ Inconclusive	PR description contains technical details but is missing key template sections required by the repository's PR guidelines.	Complete the missing sections: specify the bug with a detailed explanation, provide testing details (tests run, configuration, GPU results if applicable), and complete all relevant checklist items including code formatting verification.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title 'Bug fix for CFL adaptive time stepping' directly describes the main change in the pull request and clearly conveys what is being fixed.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[codeant-ai]

CodeAnt AI finished reviewing your PR.

[cubic-dev-ai]

No issues found across 1 file

[codeant-ai]

CodeAnt AI is running Incremental review

[codeant-ai]

CodeAnt AI Incremental review completed.