Initial Condition Extrusion + Chem Examples #1109
Conversation
📝 WalkthroughWalkthroughThis PR introduces a new 1D flamelet example case using Cantera, demonstrating pre-built patch initial conditions with a files_dir and file_extension pattern. Changes include .gitignore updates to untrack example IC data files, documentation revisions explaining the new scheme, and new configuration files for the flamelet case. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #1109 +/- ##
==========================================
- Coverage 44.00% 43.04% -0.96%
==========================================
Files 71 71
Lines 20293 20730 +437
Branches 1982 2002 +20
==========================================
- Hits 8929 8923 -6
- Misses 10227 10580 +353
- Partials 1137 1227 +90 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
PR Reviewer Guide 🔍Here are some key observations to aid the review process:
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
| q_prim_vf(2)%sf(i, j, 0) = q_prim_vf(2)%sf(i, j, 0) + u1c + u2c | ||
| q_prim_vf(3)%sf(i, j, 0) = v1c + v2c |
There was a problem hiding this comment.
Suggestion: Use symbolic momentum indices momxe and momye instead of hardcoded values, and correct the v component update to be additive. [general, importance: 7]
| q_prim_vf(2)%sf(i, j, 0) = q_prim_vf(2)%sf(i, j, 0) + u1c + u2c | |
| q_prim_vf(3)%sf(i, j, 0) = v1c + v2c | |
| q_prim_vf(momxe)%sf(i, j, 0) = q_prim_vf(momxe)%sf(i, j, 0) + u1c + u2c | |
| q_prim_vf(momye)%sf(i, j, 0) = q_prim_vf(momye)%sf(i, j, 0) + v1c + v2c |
|
|
||
| Setup: Only requires specifying `init_dir` and filename pattern via `zeros_default`. Grid dimensions are automatically detected from the data files. | ||
| Implementation: All variables and file handling are managed in `src/pre_process/include/ExtrusionHardcodedIC.fpp` with no manual grid configuration needed. | ||
| Setup: Only requires specifying `files_dir` and filename pattern via `file_extension`. The files are located, for example, at `examples/2D_case/IC`, and their format is `prim.XX.YY.files_extension.dat`. |
There was a problem hiding this comment.
Suggestion: Clarify the example filename format in the documentation to show a concrete example, such as prim.XX.YY.dat, instead of using file_extension as a literal part of the name. [general, importance: 4]
| Setup: Only requires specifying `files_dir` and filename pattern via `file_extension`. The files are located, for example, at `examples/2D_case/IC`, and their format is `prim.XX.YY.files_extension.dat`. | |
| Setup: Only requires specifying `files_dir` and filename pattern via `file_extension`. The files are located, for example, at `examples/2D_case/IC`, and their format is `prim.XX.YY.dat` (assuming `file_extension` is set to `.dat`). |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 57 files
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="docs/documentation/case.md">
<violation number="1" location="docs/documentation/case.md:254">
P3: The filename example uses `files_extension` while the text defines the key as `file_extension`. This mismatch is likely a typo and could mislead users about the required filename pattern.</violation>
</file>
<file name="examples/1D_flamelet/sandiego.yaml">
<violation number="1" location="examples/1D_flamelet/sandiego.yaml:14">
P1: The phase element list omits carbon even though carbon-containing species are defined (CO/CO2/HCO). Add `C` to the elements list so the mechanism is valid.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
|
|
||
| Setup: Only requires specifying `init_dir` and filename pattern via `zeros_default`. Grid dimensions are automatically detected from the data files. | ||
| Implementation: All variables and file handling are managed in `src/pre_process/include/ExtrusionHardcodedIC.fpp` with no manual grid configuration needed. | ||
| Setup: Only requires specifying `files_dir` and filename pattern via `file_extension`. The files are located, for example, at `examples/2D_case/IC`, and their format is `prim.XX.YY.files_extension.dat`. |
There was a problem hiding this comment.
P3: The filename example uses files_extension while the text defines the key as file_extension. This mismatch is likely a typo and could mislead users about the required filename pattern.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At docs/documentation/case.md, line 254:
<comment>The filename example uses `files_extension` while the text defines the key as `file_extension`. This mismatch is likely a typo and could mislead users about the required filename pattern.</comment>
<file context>
@@ -251,8 +251,8 @@ The code provides three pre-built patches for dimensional extrusion of initial c
-Setup: Only requires specifying `init_dir` and filename pattern via `zeros_default`. Grid dimensions are automatically detected from the data files.
-Implementation: All variables and file handling are managed in `src/pre_process/include/ExtrusionHardcodedIC.fpp` with no manual grid configuration needed.
+Setup: Only requires specifying `files_dir` and filename pattern via `file_extension`. The files are located, for example, at `examples/2D_case/IC`, and their format is `prim.XX.YY.files_extension.dat`.
+Implementation: All variables and file handling are managed in the `case.py` file of the simulation.
Usage: Ideal for initializing simulations from lower-dimensional solutions, enabling users to add perturbations or modifications to the base extruded fields for flow instability studies.
</file context>
| Setup: Only requires specifying `files_dir` and filename pattern via `file_extension`. The files are located, for example, at `examples/2D_case/IC`, and their format is `prim.XX.YY.files_extension.dat`. | |
| Setup: Only requires specifying `files_dir` and filename pattern via `file_extension`. The files are located, for example, at `examples/2D_case/IC`, and their format is `prim.XX.YY.file_extension.dat`. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @.gitignore:
- Line 26: The negation pattern "!examples/**/IC/*.dat" is placed before the
broader ignore "examples/**/*.dat" so it is ineffective; relocate the line
containing "!examples/**/IC/*.dat" to after the broader ignore pattern
"examples/**/*.dat" (or after the block of similar negations) and remove the
original occurrence so the IC .dat files are actually un-ignored.
🧹 Nitpick comments (5)
examples/1D_flamelet/sandiego.yaml (1)
14-15: Unused species definitions present in the file.The phase declares
elements: [H, O, N]and species list contains only H/O/N compounds, but the file also defines CO, CO2, and HCO (lines 160-211) which require element C. These species are not in the phase's species list and will be ignored by Cantera.This appears to be residual content from the original San Diego mechanism. Consider removing the unused CO, CO2, and HCO species definitions (lines 160-211) for clarity, or add element C and include these species if they're needed.
examples/1D_flamelet/case.py (4)
3-4: Remove unused imports.
argparseandmathare imported but never used in this file.Suggested fix
#!/usr/bin/env python3 import json -import argparse -import math import os import cantera as ct
10-11: Cantera solutionsol_Lis created but never used.The Cantera
Solutionobject is instantiated and configured with temperature, pressure, and composition, but it's not used to derive any values for the case dictionary. If this is intentional scaffolding for future use, consider adding a comment. Otherwise, these lines can be removed.Suggested fix if not needed
current_dir = os.path.dirname(os.path.abspath(__file__)) ctfile = "sandiego.yaml" -sol_L = ct.Solution(ctfile) -sol_L.TPX = 300, 8000, "O2:2,N2:2,H2O:5" L = 0.016
18-18: Unused variableSAVE_COUNT.
SAVE_COUNT = 1000is defined but never referenced.NS(line 19) is used fort_step_saveinstead.Suggested fix
NT = int(Tend / dt) -SAVE_COUNT = 1000 NS = 1000
63-66: String values for numeric parameters may cause issues.
vel(1)andalpha_rho(1)are set as string"0"and"1"respectively. While this may work for analytical definitions (as per docs), these appear to be simple constant values. Consider using numeric literals for consistency with other numeric parameters in the case dictionary.Suggested fix
- "patch_icpp(1)%vel(1)": "0", + "patch_icpp(1)%vel(1)": 0, "patch_icpp(1)%pres": 1.01325e5, "patch_icpp(1)%alpha(1)": 1, - "patch_icpp(1)%alpha_rho(1)": "1", + "patch_icpp(1)%alpha_rho(1)": 1,
|
|
||
| .DS_Store | ||
|
|
||
| !examples/**/IC/*.dat |
There was a problem hiding this comment.
Gitignore negation pattern is in wrong position and will be ineffective.
The negation !examples/**/IC/*.dat on line 26 appears before the broader ignore pattern examples/**/*.dat on line 47. In .gitignore, later patterns override earlier ones, so IC .dat files will still be ignored.
Move this line after line 47 (or after line 64 where similar negations exist) to ensure IC data files are properly tracked.
Suggested fix
Move line 26 to after line 64, grouping it with similar negation patterns:
examples/**/*.f90
!examples/3D_lag_bubbles_shbubcollapse/input/lag_bubbles.dat
!examples/3D_lag_bubbles_bubblescreen/input/lag_bubbles.dat
+!examples/**/IC/*.dat
workloads/And remove the current line 26.
🤖 Prompt for AI Agents
In @.gitignore at line 26, The negation pattern "!examples/**/IC/*.dat" is
placed before the broader ignore "examples/**/*.dat" so it is ineffective;
relocate the line containing "!examples/**/IC/*.dat" to after the broader ignore
pattern "examples/**/*.dat" (or after the block of similar negations) and remove
the original occurrence so the IC .dat files are actually un-ignored.
|
CodeAnt AI is reviewing your PR. Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
codeant-ai
bot
added
the
size:XL
Nitpicks 🔍
|
![codeant-ai[bot]](https://avatars.githubusercontent.com/in/646884?s=60&v=4){kind=small}
| "patch_icpp(1)%vel(1)": "0", | ||
| "patch_icpp(1)%pres": 1.01325e5, | ||
| "patch_icpp(1)%alpha(1)": 1, | ||
| "patch_icpp(1)%alpha_rho(1)": "1", |
There was a problem hiding this comment.
Suggestion: Several fields in the case dict that represent numeric configuration values are provided as strings (for example patch_icpp(1)%vel(1) and patch_icpp(1)%alpha_rho(1)), which can cause downstream config parsers to treat them as strings and fail numeric operations; make these entries numeric (remove the quotes). [type error]
Severity Level: Major ⚠️
- ❌ Simulation config fields parsed as wrong types.
- ⚠️ Downstream numeric operations may misbehave.| "patch_icpp(1)%vel(1)": "0", | |
| "patch_icpp(1)%pres": 1.01325e5, | |
| "patch_icpp(1)%alpha(1)": 1, | |
| "patch_icpp(1)%alpha_rho(1)": "1", | |
| "patch_icpp(1)%vel(1)": 0, | |
| "patch_icpp(1)%alpha_rho(1)": 1, |
Steps of Reproduction ✅
1. Run the example script to emit JSON: `python examples/1D_flamelet/case.py`; the script
prints the `case` dict as JSON.
2. Parse the output JSON (e.g., `json.loads(...)`) and inspect keys "patch_icpp(1)%vel(1)"
and "patch_icpp(1)%alpha_rho(1)" defined at lines 63 and 66; both values are strings ("0"
and "1").
3. If a downstream consumer expects numeric types (e.g., performing arithmetic on these
config entries), it will receive strings and either convert them or raise a type-related
error in the consumer.
4. Observed result: mis-typed configuration values in produced JSON; behavior depends on
downstream code but can lead to incorrect numeric operations.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** examples/1D_flamelet/case.py
**Line:** 63:66
**Comment:**
*Type Error: Several fields in the `case` dict that represent numeric configuration values are provided as strings (for example `patch_icpp(1)%vel(1)` and `patch_icpp(1)%alpha_rho(1)`), which can cause downstream config parsers to treat them as strings and fail numeric operations; make these entries numeric (remove the quotes).
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.| ctfile = "h2o2.yaml" | ||
| sol_L = ct.Solution(ctfile) | ||
| sol_L.TPX = 300, 101325, "H:1" |
There was a problem hiding this comment.
Suggestion: Creating the Cantera Solution at module import time will raise an exception (and perform heavy initialization) if the Cantera input file is missing or inaccessible and also prevents importing this module in contexts that only need metadata; defer construction of the Solution and use an explicit factory function (or handle errors) so imports don't fail and initialization is explicit. [possible bug]
Severity Level: Major ⚠️
- ❌ Example script fails when cantera file missing.
- ⚠️ Importing examples breaks docs/tests requiring introspection.
- ⚠️ CI that imports examples can error during import.| ctfile = "h2o2.yaml" | |
| sol_L = ct.Solution(ctfile) | |
| sol_L.TPX = 300, 101325, "H:1" | |
| ctfile = os.path.join(current_dir, "h2o2.yaml") | |
| def make_solution(): | |
| # Create and return a Cantera Solution when actually needed; | |
| # avoid side effects at import time and surface file errors at call site. | |
| sol = ct.Solution(ctfile) | |
| sol.TPX = (300, 101325, "H:1") | |
| return sol |
Steps of Reproduction ✅
1. Run the example script directly: execute `python
examples/2D_premixed_landau_insta/case.py`. The interpreter runs top-level code starting
at `examples/2D_premixed_landau_insta/case.py:9` which sets `current_dir` and `ctfile`,
then calls `ct.Solution(ctfile)` at `.../case.py:11`.
2. If `examples/2D_premixed_landau_insta/h2o2.yaml` is missing or unreadable,
`ct.Solution(ctfile)` at `.../case.py:11` raises a Cantera/FileNotFound or parsing
exception immediately during import/run.
3. Import the module from another script (for metadata only): `from
examples.2D_premixed_landau_insta import case` triggers the same execution path and raises
at `.../case.py:11`, preventing simple introspection or documentation builds that import
example modules.
4. Observed behavior: import/run fails with an exception from Cantera at the call site
(`case.py:11`), and no lazy construction allows callers to handle or skip Cantera
initialization.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** examples/2D_premixed_landau_insta/case.py
**Line:** 10:12
**Comment:**
*Possible Bug: Creating the Cantera Solution at module import time will raise an exception (and perform heavy initialization) if the Cantera input file is missing or inaccessible and also prevents importing this module in contexts that only need metadata; defer construction of the Solution and use an explicit factory function (or handle errors) so imports don't fail and initialization is explicit.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.| u2c = cvortex*((y_cc(j) - y2c))*exp(-r2c/(2.0_wp*rvortex**2.0_wp)) | ||
| v2c = -cvortex*((x_cc(i) - x2c))*exp(-r2c/(2.0_wp*rvortex**2.0_wp)) | ||
| q_prim_vf(2)%sf(i, j, 0) = q_prim_vf(2)%sf(i, j, 0) + u1c + u2c | ||
| q_prim_vf(3)%sf(i, j, 0) = v1c + v2c |
There was a problem hiding this comment.
Suggestion: The v-component assignment overwrites any previously-initialized momentum instead of adding the vortex contribution; this will drop prior velocity values for that component. Change the assignment to add to the existing field value (consistent with the u-component update). [logic error]
Severity Level: Critical 🚨
- ❌ case 271 initial v-velocity overwritten on initialization.
- ⚠️ Premixed flame vortices example yields wrong initial flow.
- ⚠️ Regression for tests referencing 2-vortex example.| q_prim_vf(3)%sf(i, j, 0) = v1c + v2c | |
| q_prim_vf(3)%sf(i, j, 0) = q_prim_vf(3)%sf(i, j, 0) + v1c + v2c |
Steps of Reproduction ✅
1. Set up a simulation that uses hard-coded IC case 271: in the code path where select
case (patch_icpp(patch_id)%hcid) is evaluated in src/common/include/2dHardcodedIC.fpp,
ensure patch_icpp(patch_id)%hcid == 271 so execution enters the case block beginning at
line 211–216 (case (271), HardcodedReadValues() call at line 213).
2. HardcodedReadValues() (called at line 213) populates baseline q_prim_vf fields for the
patch; immediately after, the vortex contribution is computed at lines 223–227
(u1c,v1c,u2c,v2c). These lines exist in src/common/include/2dHardcodedIC.fpp:223–227.
3. The code writes the u-component by adding into the existing field (line 228), but
writes the v-component with a direct assignment (line 229). Run initialization and inspect
q_prim_vf(3)%sf(i,j,0) after initialization: it will equal v1c+v2c, overwriting any
pre-existing value set by HardcodedReadValues() or other initializations.
4. Expected outcome: prior v-momentum populated by HardcodedReadValues() is lost. To
verify, place a breakpoint or print after line 229 and confirm q_prim_vf(3)%sf differs
from the pre-call value. This demonstrates the overwrite at
src/common/include/2dHardcodedIC.fpp:229.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** src/common/include/2dHardcodedIC.fpp
**Line:** 229:229
**Comment:**
*Logic Error: The v-component assignment overwrites any previously-initialized momentum instead of adding the vortex contribution; this will drop prior velocity values for that component. Change the assignment to add to the existing field value (consistent with the u-component update).
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.| real(wp) :: x_len, x_step, y_len, y_step | ||
| real(wp) :: dummy_x, dummy_y, dummy_z, x0, y0 | ||
| integer :: global_offset_x, global_offset_y ! MPI subdomain offset | ||
| real(wp) :: delta_x, delta_y | ||
| character(len=100), dimension(sys_size) :: fileNames ! Arrays to store all data from files | ||
| character(len=150), dimension(sys_size) :: fileNames ! Arrays to store all data from files |
There was a problem hiding this comment.
Suggestion: Elements of fileNames are declared with fixed length 150 and the code constructs full path strings by concatenating directory, filename, timestep and extension; long files_dir or file_extension values can cause truncation of the constructed filename leading to failed opens. Increase the per-element length to a safe value (or make the array allocatable with appropriate lengths) to avoid silent truncation. [possible bug]
Severity Level: Major ⚠️
- ❌ File open fails aborting initialization.
- ⚠️ Initial condition extrusion file reading.
- ⚠️ CI/test runs using long paths.| character(len=150), dimension(sys_size) :: fileNames ! Arrays to store all data from files | |
| character(len=512), dimension(sys_size) :: fileNames ! Arrays to store all data from files |
Steps of Reproduction ✅
1. Inspect the constructor of file path strings in the same macro: at the file-naming loop
(do f = 1, max_files) the code assigns fileNames(f) with
trim(files_dir)//"/"//"prim."//... (this assignment appears in the macro body where
fileNames is used to open files).
2. Set up a test case where files_dir or file_extension are long (for example a deeply
nested init directory or long timestep extension). Build and run the program that calls
HardcodedReadValues() so the macro executes the do f = 1, max_files loop and constructs
fileNames(f).
3. The assignment at that line will silently truncate to 150 characters if the
concatenated path exceeds length 150; afterwards the code calls open(newunit=unit2,
file=trim(fileNames(1)), ...) and open will fail because the truncated path is not a real
file path, leading to the s_mpi_abort("Error opening file: "//trim(fileNames(1))) path in
the macro.
4. Observe immediate program abort during initial-condition file reads caused by truncated
fileNames entries. This is reproducible by providing long directory or extension strings
and running any input that exercises HardcodedReadValues().Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** src/common/include/ExtrusionHardcodedIC.fpp
**Line:** 45:45
**Comment:**
*Possible Bug: Elements of `fileNames` are declared with fixed length 150 and the code constructs full path strings by concatenating directory, filename, timestep and extension; long `files_dir` or `file_extension` values can cause truncation of the constructed filename leading to failed opens. Increase the per-element length to a safe value (or make the array allocatable with appropriate lengths) to avoid silent truncation.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.| call MPI_BCAST(files_dir, len(files_dir), MPI_CHARACTER, 0, MPI_COMM_WORLD, ierr) | ||
| call MPI_BCAST(file_extension, len(file_extension), MPI_CHARACTER, 0, MPI_COMM_WORLD, ierr) |
There was a problem hiding this comment.
Suggestion: Using LEN(...) will broadcast the entire fixed-length character buffer including trailing spaces which can introduce unexpected padding in receivers and break subsequent string comparisons; use LEN_TRIM(...) to send only the meaningful characters. [logic error]
Severity Level: Major ⚠️
- ❌ Pre-processing file path and extension matching fails.
- ⚠️ Model/file I/O may silently use incorrect names.| call MPI_BCAST(files_dir, len(files_dir), MPI_CHARACTER, 0, MPI_COMM_WORLD, ierr) | |
| call MPI_BCAST(file_extension, len(file_extension), MPI_CHARACTER, 0, MPI_COMM_WORLD, ierr) | |
| call MPI_BCAST(files_dir, len_trim(files_dir), MPI_CHARACTER, 0, MPI_COMM_WORLD, ierr) | |
| call MPI_BCAST(file_extension, len_trim(file_extension), MPI_CHARACTER, 0, MPI_COMM_WORLD, ierr) |
Steps of Reproduction ✅
1. Run the code under MPI so the pre-processing phase executes (module m_mpi_proxy,
subroutine s_mpi_bcast_user_inputs) — see file src/pre_process/m_mpi_proxy.fpp around
lines 41-44. The MPI_BCAST calls for case_dir, files_dir, file_extension are executed at
lines 42-44.
2. On rank 0 provide a files_dir and file_extension that are shorter than their declared
fixed length (e.g. 'data/' and '.chem'). These are set by the input reader before
s_mpi_bcast_user_inputs is called.
3. When the two MPI_BCAST calls at lines 43 and 44 run they currently use len(...) and
will transmit the full declared character length including trailing spaces to all ranks.
4. On receiver ranks the received strings contain trailing spaces; any subsequent exact
string comparisons or filename concatenation in the pre-processing/path resolution will
observe mismatches (e.g. extension checks or file-open path lookups will fail or
misroute).Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** src/pre_process/m_mpi_proxy.fpp
**Line:** 43:44
**Comment:**
*Logic Error: Using LEN(...) will broadcast the entire fixed-length character buffer including trailing spaces which can introduce unexpected padding in receivers and break subsequent string comparisons; use LEN_TRIM(...) to send only the meaningful characters.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.| @@ -153,7 +153,8 @@ contains | |||
| viscous, bubbles_lagrange, bc_x, bc_y, bc_z, num_bc_patches, & | |||
| patch_bc, Bx0, relativity, cont_damage, igr, igr_order, & | |||
| down_sample, recon_type, muscl_order, & | |||
| simplex_perturb, simplex_params, fft_wrt | |||
| simplex_perturb, simplex_params, fft_wrt, & | |||
There was a problem hiding this comment.
Suggestion: The identifier fft_wrt looks like a probable typo (perhaps intended fft_write or fft_wrt_mode) — a misspelled namelist name will not match the declaration in the parameter module and will trigger a namelist read error. Rename the namelist entry to the exact variable name used in m_global_parameters (or correct the variable name in the parameters module) so the namelist variable matches the declaration. [possible bug]
Severity Level: Major ⚠️
- ❌ Namelist read failing aborts all pre-process runs.
- ⚠️ Input parsing fragile to naming mismatches.| simplex_perturb, simplex_params, fft_wrt, & | |
| simplex_perturb, simplex_params, fft_write, & |
Steps of Reproduction ✅
1. On proc_rank == 0, s_initialize_mpi_domain calls s_read_input_file
(src/pre_process/m_start_up.fpp), which contains the namelist block at lines 153-157 that
includes the identifier fft_wrt (line 156).
2. If the variable in m_global_parameters is actually named fft_write (or another
different spelling), the read (1, NML=user_inputs, iostat=iostatus) will not map the
namelist record to a declared variable and may result in iostat /= 0.
3. After the read, the code checks iostat and, when non-zero, prints 'Invalid line in
namelist' and triggers s_mpi_abort, terminating the pre-process (this path is executed for
any run that reads pre_process.inp).
4. Reproducing: run the usual pre-process start (the initialization path that invokes
s_read_input_file) with a pre_process.inp that contains values; observe immediate abort
when the namelist contains an entry that doesn't match the declared variable name in
m_global_parameters.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** src/pre_process/m_start_up.fpp
**Line:** 156:156
**Comment:**
*Possible Bug: The identifier `fft_wrt` looks like a probable typo (perhaps intended `fft_write` or `fft_wrt_mode`) — a misspelled namelist name will not match the declaration in the parameter module and will trigger a namelist read error. Rename the namelist entry to the exact variable name used in `m_global_parameters` (or correct the variable name in the parameters module) so the namelist variable matches the declaration.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.| f'1D -> Chemistry -> Flamelet','examples/1D_flamelet/case.py', | ||
| mods={ | ||
| **common_mods | ||
| }, | ||
| override_tol= 10**(-10) |
There was a problem hiding this comment.
Suggestion: The new Flamelet test is a 1D chemistry example but the call to define_case_f does not pass any CLI/ndim arguments like other nD chemistry cases; if the example's case.py expects an explicit --ndim argument it may default to the wrong dimensionality. Pass ['--ndim','1'] (or the expected CLI arguments) as the third positional parameter to ensure it runs as 1D. [logic error]
Severity Level: Major ⚠️
- ❌ Chemistry Flamelet example may run wrong dimension.
- ⚠️ CI chemistry tests may be invalidated.
- ⚠️ Example regression/validation confusing developers.| f'1D -> Chemistry -> Flamelet','examples/1D_flamelet/case.py', | |
| mods={ | |
| **common_mods | |
| }, | |
| override_tol= 10**(-10) | |
| f'1D -> Chemistry -> Flamelet','examples/1D_flamelet/case.py', ['--ndim','1'], | |
| mods={ | |
| **common_mods | |
| }, | |
| override_tol= 1e-10 |
Steps of Reproduction ✅
1. Inspect chemistry_cases() in toolchain/mfc/test/cases.py: the earlier nD chemistry
examples pass ['--ndim', str(ndim)] (see the Perfect Reactor loop in the same function),
but the new Flamelet entry at toolchain/mfc/test/cases.py:1087-1093 omits CLI args.
2. Run list_cases() (entrypoint in the same file) so chemistry_cases() is executed and the
define_case_f call for Flamelet is appended to cases.
3. When the test harness later executes this example, the example's case.py
(examples/1D_flamelet/case.py) will receive no explicit --ndim value from define_case_f;
if that case.py expects an --ndim argument (pattern used by other nD examples), it will
fall back to its own default or internal detection and may run at a different
dimensionality.
4. Reproduce: compare behavior of examples/nD_perfect_reactor/case.py calls (which pass
['--ndim', str(ndim)]) against examples/1D_flamelet/case.py run created by the current
define_case_f entry; observe mismatched dimensional configuration or different runtime
parameters reported by the example.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** toolchain/mfc/test/cases.py
**Line:** 1088:1092
**Comment:**
*Logic Error: The new Flamelet test is a 1D chemistry example but the call to `define_case_f` does not pass any CLI/ndim arguments like other nD chemistry cases; if the example's `case.py` expects an explicit `--ndim` argument it may default to the wrong dimensionality. Pass ['--ndim','1'] (or the expected CLI arguments) as the third positional parameter to ensure it runs as 1D.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
|
CodeAnt AI finished reviewing your PR. |
1 participant
User description
User description
Description
Please include a summary of the changes and the related issue(s) if they exist.
Please also include relevant motivation and context.
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) [optional]
Type of change
Please delete options that are not relevant.
Scope
How Has This Been Tested?
Please describe the tests that you ran to verify your changes.
Provide instructions so we can reproduce.
Please also list any relevant details for your test configuration
Flame_Vortex_Interaction.mp4
Flame_Insta.mp4
Checklist
docs/)examples/that demonstrate my new feature performing as expected.They run to completion and demonstrate "interesting physics"
./mfc.sh formatbefore committing my codeIf your code changes any code source files (anything in
src/simulation)To make sure the code is performing as expected on GPU devices, I have:
nvtxranges so that they can be identified in profiles./mfc.sh run XXXX --gpu -t simulation --nsys, and have attached the output file (.nsys-rep) and plain text results to this PR./mfc.sh run XXXX --gpu -t simulation --rsys --hip-trace, and have attached the output file and plain text results to this PR.PR Type
Enhancement, Tests
Description
Simplified IC extrusion workflow with configurable file paths and extensions
Added two new 2D premixed flame chemistry examples with vortex and Landau instability
Implemented hardcoded IC cases 271 and 272 for flame-vortex interaction and perturbed flame front
Added 1D flamelet chemistry example with San Diego reaction mechanism
Updated global parameters to support flexible IC file directory configuration
Diagram Walkthrough
File Walkthrough
3 files
Added hardcoded IC cases for flame-vortex and perturbed flameRefactored extrusion to use configurable file paths and extensionsAdded global parameters for IC file directory and extension3 files
Added file directory and extension parameters to startup routineAdded MPI broadcast for file directory and extension parametersAdded file directory and extension parameter type definitions6 files
New 2D premixed flame with vortex interaction example caseNew 2D premixed flame with Landau instability example caseNew 1D flamelet chemistry example case configurationSan Diego hydrogen-oxygen reaction mechanism for flamelet caseAdded new chemistry example cases to test suiteTest metadata for extrusion feature validation1 files
Updated IC extrusion documentation with new parameter names1 files
Summary by CodeRabbit
New Features
Documentation
Chores
✏️ Tip: You can customize this high-level summary in your review settings.
CodeAnt-AI Description
Simplify and extend initial-condition extrusion and add chemistry example cases
What Changed
Impact
✅ Clearer IC file configuration for extrusion✅ Shorter setup to run 1D->2D/2D->2D extrusion cases✅ Reproducible premixed-flame instability examples added💡 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:
This lets you have a chat with CodeAnt AI about your pull request, making it easier to understand and improve your code.
Example
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:
This helps CodeAnt AI learn and adapt to your team's coding style and standards.
Example
Retrigger review
Ask CodeAnt AI to review the PR again, by typing:
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.