Isothermal Boundary Conditions for Reacting Flows#1361
Open
DimAdam-01 wants to merge 20 commits intoMFlowCode:masterfrom
Open
Isothermal Boundary Conditions for Reacting Flows#1361DimAdam-01 wants to merge 20 commits intoMFlowCode:masterfrom
DimAdam-01 wants to merge 20 commits intoMFlowCode:masterfrom
Conversation
added 16 commits
April 1, 2026 22:43
DimAdam-01
changed the title
Member
Code reviewFound 7 issues:
MFC/src/common/m_boundary_common.fpp Lines 625 to 630 in 87edb94
MFC/src/common/m_boundary_common.fpp Lines 289 to 294 in 87edb94
MFC/src/common/m_boundary_common.fpp Lines 896 to 901 in 87edb94
MFC/src/pre_process/m_data_output.fpp Lines 86 to 92 in 87edb94
MFC/src/common/m_mpi_common.fpp Lines 816 to 824 in 87edb94
MFC/src/common/m_mpi_common.fpp Lines 820 to 824 in 87edb94
MFC/src/simulation/m_global_parameters.fpp Lines 766 to 772 in 87edb94 🤖 Generated with Claude Code - If this code review was useful, please react with 👍. Otherwise, react with 👎. |
Claude Code ReviewHead SHA: 87edb94 Files changed:
Findings1. Hardcoded wall temperature in
|
added 2 commits
April 9, 2026 23:41
Contributor
Code Review by Qodo
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Contributor
Code Review by Qodo
|
Contributor
📝 WalkthroughWalkthroughThis pull request adds support for chemistry-specific isothermal boundary conditions by introducing a separate temperature scalar field ( 🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/simulation/m_rhs.fpp (1)
582-589:⚠️ Potential issue | 🔴 CriticalPass
q_T_sfthrough the IGR communication path as well.Line 588 fixes the finite-volume branch, but when
igr = .true.the earliers_populate_variables_buffers(...)call still executes withoutq_T_sf. That leaves the chemistry temperature field out of the boundary/MPI population path for IGR cases, so isothermal-wall chemistry runs can still fail or use stale temperature halos.Suggested fix
if (igr .or. dummy) then call nvtxStartRange("RHS-COMMUNICATION") - call s_populate_variables_buffers(bc_type, q_cons_vf, pb_in, mv_in) + call s_populate_variables_buffers(bc_type, q_cons_vf, pb_in, mv_in, q_T_sf) call nvtxEndRange end ifBased on learnings: Applies to src/{common,simulation}/**/*.{fpp,f90} : Ensure MPI pack/unpack logic is updated consistently across all sweep directions (e.g., x, y, z dimensions).
src/common/m_mpi_common.fpp (1)
499-512:⚠️ Potential issue | 🔴 CriticalGuard
q_T_sfwithpresent(...)before using it.
q_T_sfis optional here, but the new chemistry-diffusion path is enabled from global flags alone. Every added pack/unpack block below dereferencesq_T_sf%sf(...), so any caller that omits the actual argument will hit undefined behavior.💡 Suggested fix
- else if (chemistry .and. chem_params%diffusion) then + else if (chemistry .and. chem_params%diffusion .and. present(q_T_sf)) then v_size = nVar + 1 buffer_counts = (/buff_size*v_size*(n + 1)*(p + 1), buff_size*v_size*(m + 2*buff_size + 1)*(p + 1), & & buff_size*v_size*(m + 2*buff_size + 1)*(n + 2*buff_size + 1)/)+ if (chemistry .and. chem_params%diffusion .and. .not. present(q_T_sf)) then + call s_mpi_abort('q_T_sf must be present when chemistry diffusion halo exchange is enabled') + end ifAlso applies to: 525-528
src/pre_process/m_data_output.fpp (1)
87-92:⚠️ Potential issue | 🔴 CriticalPass
q_T_sfthrough theigrboundary-output path too.Only the non-
igrcalls were updated. In chemistry/isothermal cases withigr = T, the boundary writers still get called withoutq_T_sf, which leaves the downstream BC packing path without the temperature field.💡 Suggested fix
if (bc_io) then if (igr) then - call s_write_serial_boundary_condition_files(q_cons_vf, bc_type, t_step_dir, old_grid) + call s_write_serial_boundary_condition_files(q_cons_vf, bc_type, t_step_dir, old_grid, q_T_sf) else call s_write_serial_boundary_condition_files(q_prim_vf, bc_type, t_step_dir, old_grid, q_T_sf) end if end ifif (bc_io) then if (igr) then - call s_write_parallel_boundary_condition_files(q_cons_vf, bc_type) + call s_write_parallel_boundary_condition_files(q_cons_vf, bc_type, q_T_sf) else call s_write_parallel_boundary_condition_files(q_prim_vf, bc_type, q_T_sf) end if end ifAlso applies to: 565-570
🧹 Nitpick comments (1)
toolchain/mfc/case_validator.py (1)
1298-1298: Renamedirto avoid Ruff A001.Using
dirhere shadows the Python builtin and already triggers the linter.directionoraxiswould keep this clean.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: dea48334-e1e4-4f3b-b300-e56f569f927b
📒 Files selected for processing (31)
CMakeLists.txtdocs/documentation/case.mdexamples/2D_Thermal_Flatplate/case.pysrc/common/include/1dHardcodedIC.fppsrc/common/include/2dHardcodedIC.fppsrc/common/m_boundary_common.fppsrc/common/m_chemistry.fppsrc/common/m_derived_types.fppsrc/common/m_mpi_common.fppsrc/post_process/m_global_parameters.fppsrc/post_process/m_mpi_proxy.fppsrc/post_process/m_start_up.fppsrc/pre_process/m_data_output.fppsrc/pre_process/m_global_parameters.fppsrc/pre_process/m_initial_condition.fppsrc/pre_process/m_mpi_proxy.fppsrc/pre_process/m_perturbation.fppsrc/pre_process/m_start_up.fppsrc/simulation/m_data_output.fppsrc/simulation/m_global_parameters.fppsrc/simulation/m_mpi_proxy.fppsrc/simulation/m_rhs.fppsrc/simulation/m_start_up.fpptests/4587725A/golden-metadata.txttests/4587725A/golden.txttests/8E56ACC1/golden-metadata.txttests/8E56ACC1/golden.txttoolchain/mfc/case_validator.pytoolchain/mfc/params/definitions.pytoolchain/mfc/params/descriptions.pytoolchain/mfc/test/cases.py
Comment on lines
+525
to
531
| else if (chemistry .and. chem_params%diffusion) then | ||
| v_size = nVar + 1 | ||
| buffer_counts = (/buff_size*v_size*(n + 1)*(p + 1), buff_size*v_size*(m + 2*buff_size + 1)*(p + 1), & | ||
| & buff_size*v_size*(m + 2*buff_size + 1)*(n + 2*buff_size + 1)/) | ||
| else | ||
| v_size = nVar | ||
| buffer_counts = (/buff_size*v_size*(n + 1)*(p + 1), buff_size*v_size*(m + 2*buff_size + 1)*(p + 1), & |
Contributor
There was a problem hiding this comment.
1. Mpi q_t_sf presence ignored 🐞 Bug ☼ Reliability
s_mpi_sendrecv_variables_buffers packs/unpacks q_T_sf whenever `chemistry .and. chem_params%diffusion is true, but q_T_sf` is an optional argument and is dereferenced without present() checks. If any caller omits q_T_sf (currently true in several places), MPI buffer packing/unpacking will read/write through an absent optional argument.
Agent Prompt
### Issue description
MPI send/recv for ghost buffers packs/unpacks temperature (`q_T_sf`) based solely on global flags (`chemistry` and `chem_params%diffusion`) even though `q_T_sf` is an optional dummy argument. This can dereference an absent optional argument and crash.
### Issue Context
Some code paths call `s_mpi_sendrecv_variables_buffers` (via `s_populate_variables_buffers`) without providing `q_T_sf` (e.g., conservative-only communications and downsample paths). The MPI routine must not assume `q_T_sf` is present.
### Fix Focus Areas
- src/common/m_mpi_common.fpp[499-593]
- src/common/m_mpi_common.fpp[564-624]
- src/common/m_mpi_common.fpp[625-713]
- src/common/m_mpi_common.fpp[756-974]
### What to change
1. Introduce a local logical like `has_T = present(q_T_sf) .and. chemistry .and. chem_params%diffusion`.
2. Use `has_T` (not just chemistry/diffusion) to:
- set `v_size` (nVar vs nVar+1)
- compute `buffer_counts`
- pack `buff_send` temperature slot
- unpack into `q_T_sf`
3. Optionally add a defensive abort if `chemistry .and. chem_params%diffusion` is true in a code path that *requires* temperature exchange but `present(q_T_sf)` is false.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #1361 +/- ##
==========================================
- Coverage 64.88% 64.87% -0.01%
==========================================
Files 70 70
Lines 18251 18389 +138
Branches 1508 1527 +19
==========================================
+ Hits 11842 11930 +88
- Misses 5446 5503 +57
+ Partials 963 956 -7 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Summarize your changes and the motivation behind them.
This PR introduces isothermal wall boundary conditions to support accurate heat transfer modeling in chemically reacting flow simulations, specifically targeting solid fuel combustion geometries/
Fixes #(issue)
Type of change
Key Changes & Additions:
Isothermal Boundaries: Added support for isothermal_in and isothermal_out flags alongside T
wall_in and Twall_out parameters across all domain boundaries in m_boundary_common.fpp.
Updated the boundary condition (slip and no-slip) logic to correctly evaluate the ghost cell temperature based on the specified wall temperature and the interior domain state, driving the correct heat flux.
Testing
How did you test your changes?
Test 1: Dual Isothermal Wall.
Test 2: 2D Flow in a Channel with Isothermal walls
2D_Thermal_Plate.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