Isothermal Boundary Conditions for Reacting Flows#1361
Merged
sbryngelson merged 35 commits intoMFlowCode:masterfrom Apr 22, 2026
Merged
Isothermal Boundary Conditions for Reacting Flows#1361sbryngelson merged 35 commits intoMFlowCode:masterfrom
sbryngelson merged 35 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 |
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 —
|
Contributor
Author
|
@sbryngelson, It's ready to merge. |
sbryngelson
reviewed
Apr 21, 2026
| @: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 |
Member
There was a problem hiding this comment.
no way, we can't do this! surely there's a better and more robust way to catalogue our problem / BC sizes
sbryngelson
reviewed
Apr 21, 2026
| & 'Web', 'Ca', 'Re_inv', 'sigR', 'sigV', 'rhoRV', 'palpha_eps', & | ||
| & 'ptgalpha_eps', 'sigma', 'pi_fac', 'mixlayer_vel_coef', 'Bx0', & | ||
| & 'mixlayer_perturb_k0'] | ||
| & 'mixlayer_perturb_k0', 'bc_x%Twall_in', 'bc_x%Twall_out', & |
Member
There was a problem hiding this comment.
how about Twall%in and Twall%out like we do for BCs?
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
3 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