Isothermal Boundary Conditions for Reacting Flows#1361
Draft
DimAdam-01 wants to merge 32 commits intoMFlowCode:masterfrom
Draft
Isothermal Boundary Conditions for Reacting Flows#1361DimAdam-01 wants to merge 32 commits intoMFlowCode:masterfrom
DimAdam-01 wants to merge 32 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: 2770ad3 Files changed:
Findings1. Spurious
|
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
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #1361 +/- ##
==========================================
- Coverage 64.62% 64.60% -0.02%
==========================================
Files 71 71
Lines 18407 18566 +159
Branches 1516 1541 +25
==========================================
+ Hits 11895 11995 +100
- Misses 5555 5620 +65
+ Partials 957 951 -6 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Contributor
Code Review by Qodo
|
Contributor
Code Review by Qodo
|
Comment on lines
+45
to
+62
| integer :: i, j, sys_size_alloc | ||
|
|
||
| @:ALLOCATE(bc_buffers(1:3, 1:2)) | ||
|
|
||
| if (bc_io) then | ||
| @:ALLOCATE(bc_buffers(1, 1)%sf(1:sys_size, 0:n, 0:p)) | ||
| @:ALLOCATE(bc_buffers(1, 2)%sf(1:sys_size, 0:n, 0:p)) | ||
| sys_size_alloc = sys_size | ||
| if (chemistry) sys_size_alloc = sys_size + 1 | ||
|
|
||
| @:ALLOCATE(bc_buffers(1, 1)%sf(1:sys_size_alloc, 0:n, 0:p)) | ||
| @:ALLOCATE(bc_buffers(1, 2)%sf(1:sys_size_alloc, 0:n, 0:p)) | ||
| #:if not MFC_CASE_OPTIMIZATION or num_dims > 1 | ||
| if (n > 0) then | ||
| @:ALLOCATE(bc_buffers(2,1)%sf(-buff_size:m+buff_size,1:sys_size,0:p)) | ||
| @:ALLOCATE(bc_buffers(2,2)%sf(-buff_size:m+buff_size,1:sys_size,0:p)) | ||
| @:ALLOCATE(bc_buffers(2,1)%sf(-buff_size:m+buff_size,1:sys_size_alloc,0:p)) | ||
| @:ALLOCATE(bc_buffers(2,2)%sf(-buff_size:m+buff_size,1:sys_size_alloc,0:p)) | ||
| #:if not MFC_CASE_OPTIMIZATION or num_dims > 2 | ||
| if (p > 0) then | ||
| @:ALLOCATE(bc_buffers(3,1)%sf(-buff_size:m+buff_size,-buff_size:n+buff_size,1:sys_size)) | ||
| @:ALLOCATE(bc_buffers(3,2)%sf(-buff_size:m+buff_size,-buff_size:n+buff_size,1:sys_size)) | ||
| @:ALLOCATE(bc_buffers(3,1)%sf(-buff_size:m+buff_size,-buff_size:n+buff_size,1:sys_size_alloc)) | ||
| @:ALLOCATE(bc_buffers(3,2)%sf(-buff_size:m+buff_size,-buff_size:n+buff_size,1:sys_size_alloc)) |
Contributor
There was a problem hiding this comment.
1. bc_buffers missing @:deallocate 📘 Rule violation ☼ Reliability
This PR modifies/introduces @:ALLOCATE calls for bc_buffers storage, but the corresponding teardown still uses raw deallocate instead of @:DEALLOCATE. This violates the project requirement to pair macro allocations/deallocations, risking inconsistent resource handling across builds/backends.
Agent Prompt
## Issue description
The PR updates `@:ALLOCATE` usage for `bc_buffers` storage, but `s_finalize_boundary_common_module()` still deallocates those buffers using raw `deallocate` instead of the required `@:DEALLOCATE` macro.
## Issue Context
Compliance requires macro-based allocation/deallocation pairing for consistent resource management.
## Fix Focus Areas
- src/common/m_boundary_common.fpp[45-62]
- src/common/m_boundary_common.fpp[2326-2346]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Member
Code reviewFound 2 issues:
MFC/src/common/m_mpi_common.fpp Lines 525 to 528 in 81a48bd
MFC/src/common/m_mpi_common.fpp Lines 821 to 826 in 81a48bd Generated with Claude Code - If this code review was useful, please react with 👍. Otherwise, react with 👎. |
Member
Code review (additional findings)8 additional issues found during review, listed by estimated severity: 1. In allocate (q_T_sf%sf(0:m, 0:n, 0:p))When q_T_sf%sf(-j, k, l) = q_T_sf%sf(0, k, l) ! j=1..buff_size -> index -1..-buff_sizeSince the array lower bound is MFC/src/pre_process/m_initial_condition.fpp Lines 51 to 53 in 81a48bd 2. Validator does not require
MFC/toolchain/mfc/case_validator.py Lines 1304 to 1316 in 81a48bd 3. Adiabatic temperature ghost-cell uses reflection instead of zero-gradient extrapolation In q_T_sf%sf(-j, k, l) = q_T_sf%sf(j - 1, k, l) ! mirrorBut every other scalar in the same routines uses constant extrapolation from the boundary cell: q_prim_vf(i)%sf(-j, k, l) = q_prim_vf(i)%sf(0, k, l) ! zero-gradientAnd q_T_sf%sf(-j, k, l) = q_T_sf%sf(0, k, l)The comment on MFC/src/common/m_boundary_common.fpp Lines 858 to 863 in 81a48bd 4. Most BC routines guard temperature ghost-cell updates with: if (chemistry .and. present(q_T_sf)) thenBut if (chemistry .and. chem_params%diffusion .and. present(q_T_sf)) thenWith MFC/src/common/m_boundary_common.fpp Lines 637 to 638 in 81a48bd 5.
MFC/toolchain/mfc/case_validator.py Lines 2047 to 2049 in 81a48bd 6. Missing CLAUDE.md says "Every new parameter MUST be added in at least 3 places (4 if it has constraints)." The new 7. The description reads MFC/toolchain/mfc/params/descriptions.py Line 371 in 81a48bd 8. Double parentheses in
MFC/src/common/m_boundary_common.fpp Lines 1011 to 1013 in 81a48bd Generated with Claude Code - If this code review was useful, please react with 👍. Otherwise, react with 👎. |
sbryngelson
added
claude-full-review
Member
Code review (post-update check)Checked the changes since the previous review — the "AI Suggestion" commit ( Prior issues — statusFixed:
Still not addressed:
(Retracted: the earlier suggestion to add New issues introduced in this update1. CRITICAL —
|
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