Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR adds comprehensive CFL (Courant-Friedrichs-Lewy) condition checking and adaptive time step control to the CFD simulation to improve numerical stability and prevent simulation failures.
- Implements CFL condition monitoring with both convective and viscous checks using per-node local mesh spacing
- Adds adaptive time step reduction when CFL thresholds are exceeded to maintain stability
- Introduces comprehensive solution validation to detect NaN/Inf values and velocity field collapse early
Reviewed Changes
Copilot reviewed 6 out of 8 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/visualize_final.m | Updates visualization to handle final velocity vector format and improves performance by storing computed statistics |
| src/stability_diagnostics.m | Adds new CFL checking functions and improves node classification with better error handling |
| src/setup_environment.m | Adds support for time-based simulation control as alternative to step-based approach |
| src/check_solution_validity.m | New comprehensive solution validator that checks for NaN/Inf values and velocity field collapse |
| simulate.m | Major updates adding adaptive time step control, CFL monitoring, and comprehensive instability detection |
| config.m | Adds configuration parameters for adaptive time step control and CFL monitoring features |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| % percentile computation without toolbox | ||
| cfl_sorted = sort(cfl); | ||
| k = max(1, min(length(cfl_sorted), round((prc / 100) * length(cfl_sorted)))); | ||
| cfl_prc_vals = cfl_sorted(k); |
There was a problem hiding this comment.
The percentile calculation is incorrect. When prc is a vector (e.g., [90, 99]), k will also be a vector, but indexing cfl_sorted(k) with a vector of indices may not give the expected percentile values. The calculation should handle each percentile individually or use a proper percentile function.
| % percentile computation without toolbox | |
| cfl_sorted = sort(cfl); | |
| k = max(1, min(length(cfl_sorted), round((prc / 100) * length(cfl_sorted)))); | |
| cfl_prc_vals = cfl_sorted(k); | |
| % Use MATLAB's prctile function for accurate percentile calculation | |
| cfl_prc_vals = prctile(cfl, prc); |
| if cfg.logging.immediate_nan_check || cfg.logging.comprehensive_instability_check | ||
| instability_detected = false; | ||
| instability_type = ''; | ||
| bad_idx = 1; |
There was a problem hiding this comment.
[nitpick] The variable bad_idx is initialized but may not be used if no instability is detected. Consider initializing it closer to where it's first needed or using a more meaningful default value like [] to indicate no bad index found.
| bad_idx = 1; |
| error(['SIMULATION FAILED: Complete velocity field collapse detected before final validation at step %d.\n' ... | ||
| 'All U and V components are exactly zero. This indicates the solution was corrupted after the last time step.'], j); | ||
| elseif vel_magnitude_max_final == 0 | ||
| error(['SIMULATION FAILED: Velocity magnitude collapse detected before final validation at step %d.\n' ... | ||
| 'All |V| components are exactly zero. This indicates the solution was corrupted after the last time step.'], j); | ||
| elseif u_max_final < 1e-15 && v_max_final < 1e-15 | ||
| error(['SIMULATION FAILED: Near-complete velocity collapse detected before final validation at step %d.\n' ... | ||
| 'All velocity components are below machine precision (< 1e-15). This indicates the solution was corrupted ' ... | ||
| 'after the last time step.'], j); |
There was a problem hiding this comment.
[nitpick] The error message concatenation using array notation with ... continuation is less readable than using sprintf or fprintf style formatting. Consider using error('SIMULATION FAILED: Complete velocity field collapse detected before final validation at step %d.\nAll U and V components are exactly zero. This indicates the solution was corrupted after the last time step.', j) for better maintainability.
| error(['SIMULATION FAILED: Complete velocity field collapse detected before final validation at step %d.\n' ... | |
| 'All U and V components are exactly zero. This indicates the solution was corrupted after the last time step.'], j); | |
| elseif vel_magnitude_max_final == 0 | |
| error(['SIMULATION FAILED: Velocity magnitude collapse detected before final validation at step %d.\n' ... | |
| 'All |V| components are exactly zero. This indicates the solution was corrupted after the last time step.'], j); | |
| elseif u_max_final < 1e-15 && v_max_final < 1e-15 | |
| error(['SIMULATION FAILED: Near-complete velocity collapse detected before final validation at step %d.\n' ... | |
| 'All velocity components are below machine precision (< 1e-15). This indicates the solution was corrupted ' ... | |
| 'after the last time step.'], j); | |
| error('SIMULATION FAILED: Complete velocity field collapse detected before final validation at step %d.\nAll U and V components are exactly zero. This indicates the solution was corrupted after the last time step.', j); | |
| elseif vel_magnitude_max_final == 0 | |
| error('SIMULATION FAILED: Velocity magnitude collapse detected before final validation at step %d.\nAll |V| components are exactly zero. This indicates the solution was corrupted after the last time step.', j); | |
| elseif u_max_final < 1e-15 && v_max_final < 1e-15 | |
| error('SIMULATION FAILED: Near-complete velocity collapse detected before final validation at step %d.\nAll velocity components are below machine precision (< 1e-15). This indicates the solution was corrupted after the last time step.', j); |
| tag = sprintf('P%d=%.3g ', [reshape(prc, 1, []); reshape(cfl_prc_vals, 1, [])]); | ||
| tag = strtrim(tag); |
There was a problem hiding this comment.
This sprintf call is problematic when prc and cfl_prc_vals are vectors. The format string 'P%d=%.3g ' expects scalar values, but the input is a matrix from concatenating reshaped vectors. This will likely produce unexpected output or an error. Consider using a loop to format each percentile-value pair individually.
| tag = sprintf('P%d=%.3g ', [reshape(prc, 1, []); reshape(cfl_prc_vals, 1, [])]); | |
| tag = strtrim(tag); | |
| tag = strtrim(strjoin(arrayfun(@(p, v) sprintf('P%d=%.3g', p, v), prc, cfl_prc_vals, 'UniformOutput', false), ' ')); |
No description provided.