Extrusion v3#1349
Conversation
Review Summary by QodoAdd premixed flame IC cases and refactor extrusion workflow with configurable paths
WalkthroughsDescription• Add two new hardcoded IC cases (271, 272) for premixed flame simulations - Case 271: Premixed flame vortex interaction with dual counter-rotating vortices - Case 272: Premixed flame instability with sinusoidal front perturbation • Refactor IC extrusion workflow to use configurable files_dir and file_extension parameters - Replace hardcoded path and filename pattern with user-configurable parameters - Improve MPI communication for new parameters via broadcast • Add three reference example cases with complete configuration files - 1D flamelet case with San Diego mechanism - 2D premixed flame vortex interaction case - 2D premixed flame Landau instability case • Update parameter definitions and test suite for new IC extrusion examples Diagramflowchart LR
A["Hardcoded IC Cases<br/>271, 272"] -->|"Vortex & Instability<br/>Perturbations"| B["2D Premixed<br/>Flame Simulations"]
C["Configurable<br/>files_dir & file_extension"] -->|"Replace hardcoded<br/>init_dir"| D["Flexible IC<br/>Extrusion"]
E["Reference Examples<br/>1D, 2D cases"] -->|"Complete setup<br/>with case.py"| F["User-friendly<br/>Workflow"]
D --> B
F --> B
File Changes1. src/common/include/2dHardcodedIC.fpp
|
Code Review by Qodo
1.
|
📝 WalkthroughWalkthroughThis pull request introduces a new 1D flamelet example case and updates related documentation. A 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
.gitignore (1)
57-57: Redundant ignore pattern (no functional change).Line 57 is already covered by
examples/**/*.dat(Line 64), so this new rule is a no-op. Consider removing it to keep ignore rules minimal.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 96117184-e389-41b7-b7bb-97731b0c2155
⛔ Files ignored due to path filters (42)
examples/1D_flamelet/IC/prim.1.00.000000.datis excluded by!**/*.datexamples/1D_flamelet/IC/prim.10.00.000000.datis excluded by!**/*.datexamples/1D_flamelet/IC/prim.11.00.000000.datis excluded by!**/*.datexamples/1D_flamelet/IC/prim.12.00.000000.datis excluded by!**/*.datexamples/1D_flamelet/IC/prim.13.00.000000.datis excluded by!**/*.datexamples/1D_flamelet/IC/prim.14.00.000000.datis excluded by!**/*.datexamples/1D_flamelet/IC/prim.2.00.000000.datis excluded by!**/*.datexamples/1D_flamelet/IC/prim.3.00.000000.datis excluded by!**/*.datexamples/1D_flamelet/IC/prim.4.00.000000.datis excluded by!**/*.datexamples/1D_flamelet/IC/prim.5.00.000000.datis excluded by!**/*.datexamples/1D_flamelet/IC/prim.6.00.000000.datis excluded by!**/*.datexamples/1D_flamelet/IC/prim.7.00.000000.datis excluded by!**/*.datexamples/1D_flamelet/IC/prim.8.00.000000.datis excluded by!**/*.datexamples/1D_flamelet/IC/prim.9.00.000000.datis excluded by!**/*.datexamples/2D_premixed_flame_vortex/IC/prim.1.00.000000.datis excluded by!**/*.datexamples/2D_premixed_flame_vortex/IC/prim.10.00.000000.datis excluded by!**/*.datexamples/2D_premixed_flame_vortex/IC/prim.11.00.000000.datis excluded by!**/*.datexamples/2D_premixed_flame_vortex/IC/prim.12.00.000000.datis excluded by!**/*.datexamples/2D_premixed_flame_vortex/IC/prim.13.00.000000.datis excluded by!**/*.datexamples/2D_premixed_flame_vortex/IC/prim.14.00.000000.datis excluded by!**/*.datexamples/2D_premixed_flame_vortex/IC/prim.2.00.000000.datis excluded by!**/*.datexamples/2D_premixed_flame_vortex/IC/prim.3.00.000000.datis excluded by!**/*.datexamples/2D_premixed_flame_vortex/IC/prim.4.00.000000.datis excluded by!**/*.datexamples/2D_premixed_flame_vortex/IC/prim.5.00.000000.datis excluded by!**/*.datexamples/2D_premixed_flame_vortex/IC/prim.6.00.000000.datis excluded by!**/*.datexamples/2D_premixed_flame_vortex/IC/prim.7.00.000000.datis excluded by!**/*.datexamples/2D_premixed_flame_vortex/IC/prim.8.00.000000.datis excluded by!**/*.datexamples/2D_premixed_flame_vortex/IC/prim.9.00.000000.datis excluded by!**/*.datexamples/2D_premixed_landau_insta/IC/prim.1.00.000000.datis excluded by!**/*.datexamples/2D_premixed_landau_insta/IC/prim.10.00.000000.datis excluded by!**/*.datexamples/2D_premixed_landau_insta/IC/prim.11.00.000000.datis excluded by!**/*.datexamples/2D_premixed_landau_insta/IC/prim.12.00.000000.datis excluded by!**/*.datexamples/2D_premixed_landau_insta/IC/prim.13.00.000000.datis excluded by!**/*.datexamples/2D_premixed_landau_insta/IC/prim.14.00.000000.datis excluded by!**/*.datexamples/2D_premixed_landau_insta/IC/prim.2.00.000000.datis excluded by!**/*.datexamples/2D_premixed_landau_insta/IC/prim.3.00.000000.datis excluded by!**/*.datexamples/2D_premixed_landau_insta/IC/prim.4.00.000000.datis excluded by!**/*.datexamples/2D_premixed_landau_insta/IC/prim.5.00.000000.datis excluded by!**/*.datexamples/2D_premixed_landau_insta/IC/prim.6.00.000000.datis excluded by!**/*.datexamples/2D_premixed_landau_insta/IC/prim.7.00.000000.datis excluded by!**/*.datexamples/2D_premixed_landau_insta/IC/prim.8.00.000000.datis excluded by!**/*.datexamples/2D_premixed_landau_insta/IC/prim.9.00.000000.datis excluded by!**/*.dat
📒 Files selected for processing (17)
.gitignoredocs/documentation/case.mdexamples/1D_flamelet/case.pyexamples/1D_flamelet/sandiego.yamlexamples/2D_premixed_flame_vortex/case.pyexamples/2D_premixed_landau_insta/case.pysrc/common/include/2dHardcodedIC.fppsrc/common/include/ExtrusionHardcodedIC.fppsrc/pre_process/m_global_parameters.fppsrc/pre_process/m_mpi_proxy.fppsrc/pre_process/m_start_up.fpptests/0D1E22F3/golden-metadata.txttests/0D1E22F3/golden.txttoolchain/mfc/params/definitions.pytoolchain/mfc/params/descriptions.pytoolchain/mfc/params/namelist_parser.pytoolchain/mfc/test/cases.py
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #1349 +/- ##
==========================================
+ Coverage 60.39% 60.83% +0.44%
==========================================
Files 83 83
Lines 19854 19819 -35
Branches 2955 2952 -3
==========================================
+ Hits 11990 12057 +67
+ Misses 5863 5770 -93
+ Partials 2001 1992 -9 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Code Review by Qodo
|
| #:enddef | ||
|
|
||
| #:def HardcodedReadValues() | ||
| if (.not. files_loaded) then | ||
| max_files = merge(sys_size, sys_size - 1, num_dims == 1) | ||
| do f = 1, max_files | ||
| write (file_num_str, '(I0)') f | ||
| fileNames(f) = trim(init_dir) // "prim." // trim(file_num_str) // ".00." // zeros_default // ".dat" | ||
| fileNames(f) = trim(files_dir) // "/" // "prim." // trim(file_num_str) // ".00." // trim(file_extension) // ".dat" |
There was a problem hiding this comment.
1. files_dir undefined in simulation 📘 Rule violation ≡ Correctness
ExtrusionHardcodedIC.fpp now references files_dir and file_extension, but the simulation-side m_global_parameters/namelist/MPI broadcast do not define or distribute these parameters. This can break simulation compilation or cause rank-to-rank mismatches at runtime when using extrusion-based ICs.
Agent Prompt
## Issue description
The new parameters `files_dir` and `file_extension` are used by code included in the simulation build (`ExtrusionHardcodedIC.fpp`), but they are only declared/bound/broadcast in `src/pre_process`. Simulation must also declare them in `m_global_parameters`, read them from `simulation.inp` via the `user_inputs` namelist, and broadcast them to all ranks.
## Issue Context
`src/simulation/m_ib_patches.fpp` includes `ExtrusionHardcodedIC.fpp`, which now constructs file names using `files_dir` and `file_extension`.
## Fix Focus Areas
- src/simulation/m_global_parameters.fpp[24-35]
- src/simulation/m_start_up.fpp[84-114]
- src/simulation/m_mpi_proxy.fpp[58-70]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Code Review by Qodo
|
Claude Code ReviewPR #1349 — Extrusion v3 Critical Issues1. 2D index calculation is constant — all cells read the same data row
idx = i + 1 + global_offset_x - index_xsimplifies to 2. The offset is calculated inside 3. logical :: files_loaded = .false.Fortran 2008 §5.2.4: a local variable with an initializer implicitly has the Important Issues4. Asymmetric deallocation of
5.
6. Both construct 7. Both 2D example case files reference Suggestions
Strengths
The 2D extruded IC (hcid 270/271/272) is currently broken due to issue #1: every cell gets the same row of data, producing a uniform field. Issues #2 and #3 compound this. These must be fixed before merge. |
CI failure:
|
The hardcoded array had O/O2 swapped (positions 3-4), H2O/HO2/H2O2 in wrong order (positions 6-8), and Ar/N2 swapped (positions 9-10). Corrected to match sandiego.yaml species order: [H2, H, O2, O, OH, HO2, H2O2, H2O, N2, Ar]. Also corrected Ar mass from 39.95 to 39.948 and added _wp precision suffixes.
This reverts commit 0210586.
Cray ftn GPU-OMP uses 'target teams distribute parallel do simd' which applies SIMD vectorization with GPU-specific transcendental math. Over 50 steps of stiff H2/O2 chemistry, tiny FP differences in GPU exp/log accumulate to rel errors of ~1e-3 to ~8e-2, far exceeding any useful tolerance. AMD GPU and all CPU builds agree with the gfortran golden. Reducing to 1 step keeps FP accumulation negligible and still tests IC loading, CBC boundary conditions, and chemistry kernel invocation.
Automated Code ReviewSummary: 3 critical issues, 4 important issues. Critical Issues1. Implicit
logical :: files_loaded = .false.In Fortran, a local variable with a default initializer automatically acquires the Fix: Remove the initializer and add an explicit 2. No bounds check on
idx = i + 1 + global_offset_x - index_x
stored_values(idx, 1, f) ! no range checkIf the computational domain extends beyond the file's data coverage, or MPI subdomain offset pushes Fix: Add runtime guard: if (idx < 1 .or. idx > xRows) call s_mpi_abort("idx out of bounds in extrusion IC")Similarly for 3.
character(len=255) :: files_dirOn HPC systems with deep directory hierarchies, absolute paths (e.g., Fix: Use Important Issues4. For 1D/2D extrusion, 5. Documentation error in Line 266: "All variables and file handling are managed in the This is incorrect — file reading is done entirely in Fortran ( 6. Fortran length is 10; the description says "6 digits". Inconsistency — clarify which is the actual constraint. 7. All MPI ranks read files independently — scalability concern at HPC scale (confidence: 90%)
Suggestions
Strengths
|
Resolves conflicts with the registry-generated MPI broadcast refactor: - pre_process m_global_parameters: defaults block now comes from s_assign_common_defaults; the PR's new file_extension/files_dir defaults move to the pre-specific logistics section - pre_process m_mpi_proxy: the old manual broadcast loop is replaced by generated_bcast.fpp; files_dir/file_extension broadcasts are now emitted by the generator, which gains proper STR-scalar handling (len() + MPI_CHARACTER, generalizing the case_dir form) with a params_tests case
|
I've pushed a merge of
Your Verification: |
sbryngelson
left a comment
sbryngelson
left a comment
There was a problem hiding this comment.
Automated review
Reviewed the code changes (excluding the generated .dat IC data files). Most of the diff is data; the logic changes are concentrated in src/common/include/{Extrusion,2d}HardcodedIC.fpp and a few toolchain/mfc files.
1. src/common/include/ExtrusionHardcodedIC.fpp:99 — off-by-one grid offset / out-of-bounds read (most severe)
Removing the + x_step/2.0_wp half-cell correction from the delta_x offset calculation breaks grid alignment for the two new 2D examples.
Verified against the actual shipped .dat files: their coordinates are stored in a right-cell-boundary convention (x_coords(1) == 1×dx to 15 significant digits), not the cell-center convention (0.5×dx) the rest of the macro assumes elsewhere. For 2D_premixed_landau_insta (hcid 272, m=1279), this flips global_offset_x from 0 (old formula) to 1 (new formula, via a Fortran NINT tie-break on exactly 0.5). That shifts idx = i+1+global_offset_x by one for every cell, and at i=m=1279 produces idx=1281 — one past stored_values's allocated bound of xRows=1280. This is an out-of-bounds array read inside the unconditional @:HardcodedReadValues() data-assignment pass (crash under bounds-checked builds, undefined/garbage value otherwise).
2D_premixed_flame_vortex (hcid 271) hits the same underlying coordinate-convention mismatch with almost no rounding margin (it currently still resolves to global_offset_x=0, but just barely). Unlike case 272, case 271 uses the resulting idx-indexed values directly (density/pressure/etc. are never overwritten afterward), so any small perturbation in that margin would silently shift the entire base flow field by one grid cell.
Neither 2D example has CI coverage — both 2D_premixed_landau_insta and 2D_premixed_flame_vortex are listed in casesToSkip in toolchain/mfc/test/cases.py, so only the unaffected 1D case gets a golden-file regression test. This is worth double-checking before merge, since it may just need the half-cell term restored (or the IC-file-generation convention documented/fixed) for the 2D cases specifically.
2. src/common/include/2dHardcodedIC.fpp:310 — hardcoded domain length
The hcid=272 wave perturbation computes wave_phase using a hardcoded Ly_param = 0.00775735_wp rather than deriving it from the case's actual y_domain%end - y_domain%beg. If a user copies this example and changes the domain height (keeping hcid=272), the wave perturbation no longer completes an integer number of periods across the domain, which — with periodic y BCs — creates a discontinuity at the periodic seam. Nothing in case_validator.py's new check_ic_extrusion catches this (it only checks that files_dir/file_extension are set).
3. src/common/include/ExtrusionHardcodedIC.fpp:175 — wasted work in case 272 (minor)
@:HardcodedReadValues() unconditionally performs a full data-assignment write into every q_prim_vf(f) slot using the plain-extrusion idx mapping, for every grid cell — and case 272 immediately overwrites every one of those slots with its own wave-shifted/interpolated values a few lines later. One-time pre-processing cost, but avoidable.
4. src/common/include/2dHardcodedIC.fpp:328 — duplicate bisection logic (minor)
The hand-rolled bisection search (idx_lo/idx_hi/idx_mid) to bracket x_mapped in x_coords duplicates the very similar get_indices_from_bounds bisection already implemented in src/simulation/m_ib_patches.fpp:570-609 — the file that #:includes this one.
Generated with the help of Claude Code.
Description
Summarize your changes and the motivation behind them.
IC Extrusion: The workflow for extending Initial Conditions to higher dimensions has been simplified. The updated methodology is now more robust and user-friendly.
Documentation & Examples: To assist users with this new methodology, three new reference examples have been uploaded to the repository (one 1D case and two 2D cases)
Fixes #(issue)
Type of change
-[x] A current methodology being more user-friendly
Testing
How did you test your changes?
Flame_Vortex_Interaction.mp4
-[x] 3rd Test, 2D Premixed Flame with perturbed flame front
Flame_Insta.mp4
Checklist
See the developer guide for full coding standards.
GPU changes (expand if you modified
src/simulation/)AI code reviews
Reviews are not triggered automatically. To request a review, comment on the PR:
@coderabbitai review— incremental review (new changes only)@coderabbitai full review— full review from scratch/review— Qodo review/improve— Qodo code suggestions@claude full review— Claude full review (also triggers on PR open/reopen/ready)claude-full-review— Claude full review via label