Unity Lewis Implementation #1084
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughAdds a chemistry transport_model parameter and wires it through initialization, MPI, tooling, docs, and an example; replaces a single diffusion flux computation with two transport-model-specific branches (Mixture-Average and Unity Lewis Number) in the chemistry module. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1084 +/- ##
==========================================
- Coverage 43.99% 43.99% -0.01%
==========================================
Files 71 71
Lines 20289 20292 +3
Branches 1982 1982
==========================================
+ Hits 8927 8928 +1
- Misses 10225 10227 +2
Partials 1137 1137 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
DimAdam-01
force-pushed
the
Unity_Lewis_V2
branch
from
ca44861 to
1da8408
Compare
|
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 · |
PR Reviewer Guide 🔍Here are some key observations to aid the review process:
|
codeant-ai
bot
added
the
size:L
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 8 files
Prompt for AI agents (all 1 issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="src/common/include/ExtrusionHardcodedIC.fpp">
<violation number="1" location="src/common/include/ExtrusionHardcodedIC.fpp:178">
P1: This change ignores `global_offset_x` which is calculated for MPI subdomain positioning, and makes `idx` dead code. In parallel MPI runs, this could cause incorrect data mapping since the offset to the local subdomain is no longer considered. Either use `idx` as originally intended, or remove the unused `idx` calculation if the offset is truly not needed for 1D.</violation>
</file>
Since this is your first cubic review, here's how it works:
- cubic automatically reviews your code and comments on bugs and improvements
- Teach cubic by replying to its comments. cubic learns from your replies and gets better over time
- Ask questions if you need clarification on any suggestion
Reply to cubic to teach it or ask questions. Re-run a review with @cubic-dev-ai review this PR
Nitpicks 🔍
|
There was a problem hiding this comment.
Pull request overview
This pull request introduces Unity Lewis transport modeling to MFC's chemistry module, providing an alternative to the existing mixture-averaged transport methodology. The Unity Lewis number approximation assumes thermal diffusivity equals mass diffusivity (Le = 1), which simplifies transport calculations by using enthalpy gradients instead of species-specific diffusion coefficients. The default transport model remains mixture-averaged (transport_model = 1), with Unity Lewis available as an option (transport_model = 2).
Key Changes:
- Added
transport_modelparameter to chemistry configuration, allowing selection between mixture-averaged (1) and Unity Lewis (2) transport models - Implemented Unity Lewis transport flux calculations using enthalpy-gradient formulation and thermal diffusivity
- Propagated
transport_modelparameter through MPI broadcasts and parameter initialization across all three MFC subprograms
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
toolchain/mfc/run/case_dicts.py |
Added transport_model to chemistry parameters as an integer type for input validation |
src/simulation/m_mpi_proxy.fpp |
Added MPI broadcast for transport_model parameter to ensure consistency across processes |
src/simulation/m_global_parameters.fpp |
Initialized transport_model to default value of 1 (mixture-averaged) |
src/pre_process/m_global_parameters.fpp |
Declared chem_params and initialized transport_model to 1 in pre-processor |
src/post_process/m_global_parameters.fpp |
Declared chem_params and initialized transport_model to 1 in post-processor |
src/common/m_derived_types.fpp |
Added transport_model integer field to chemistry_parameters type |
src/common/m_chemistry.fpp |
Implemented Unity Lewis model branch with enthalpy-based diffusion flux calculation; reorganized mixture-averaged code into conditional block |
src/common/include/ExtrusionHardcodedIC.fpp |
Fixed indexing bug in 1D case to use i + 1 directly instead of unused idx variable |
![codeant-ai[bot]](https://avatars.githubusercontent.com/in/646884?s=60&v=4){kind=small}
src/common/m_chemistry.fpp
Outdated
| diffusivity_R = lambda_R/rho_R/Cp_R | ||
|
|
||
| lambda_Cell = 0.5_wp*(lambda_R + lambda_L) | ||
| diffusivity_Cell = 0.5_wp*(diffusivity_R + diffusivity_L) |
There was a problem hiding this comment.
✅ Suggestion: Inconsistent variable name for the cell diffusivity leads to use of an uninitialized variable: the code assigns to diffusivity_Cell but uses diffusivity_cell when computing Mass_Diffu_Flux. Although Fortran is case-insensitive, this mismatch is confusing and can hide real typos; make the name consistent (use a single identifier) so the value computed is the one actually used. [possible bug]
Severity Level: Critical 🚨
| diffusivity_R = lambda_R/rho_R/Cp_R | |
| lambda_Cell = 0.5_wp*(lambda_R + lambda_L) | |
| diffusivity_Cell = 0.5_wp*(diffusivity_R + diffusivity_L) | |
| diffusivity_cell = 0.5_wp*(diffusivity_R + diffusivity_L) |
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** src/common/m_chemistry.fpp
**Line:** 376:379
**Comment:**
*Possible Bug: Inconsistent variable name for the cell diffusivity leads to use of an uninitialized variable: the code assigns to `diffusivity_Cell` but uses `diffusivity_cell` when computing `Mass_Diffu_Flux`. Although Fortran is case-insensitive, this mismatch is confusing and can hide real typos; make the name consistent (use a single identifier) so the value computed is the one actually used.
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. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/common/m_chemistry.fpp (1)
174-405: Validatechem_params%transport_modeland consider de-duplicating common diffusion logicThe two-branch implementation of
s_compute_chemistry_diffusion_fluxlooks internally consistent:
- Directional offsets and
grid_spacingselection are handled uniformly viaoffsets(idir).- All scalars and work arrays used inside
$:GPU_PARALLEL_LOOPare in theprivatelist, which should avoid race conditions.- Model 1 (mixture-averaged) correctly applies a mass-conservation correction (
rho_Vic) and adds thermal conduction vialambda_Cell*dT_dxi.- Model 2 (unity Lewis) derives a scalar diffusivity from
λ/(ρ Cp)and usesdh_dxiwith a singlediffusivity_cell, which automatically keepsΣ j_k = 0forΣ Y_k = 1.Two follow-ups are worth addressing:
No guard for invalid
transport_modelvaluesIf
chem_params%transport_modelis set to anything other than1or2, the routine silently skips diffusion (the outerif (chemistry)is still true, but neither branch runs). That can produce physically wrong results without any diagnostics.Consider adding an
elsebranch that aborts clearly, for example:if (chem_params%transport_model == 1) then ! ... else if (chem_params%transport_model == 2) then ! ... else call s_mpi_abort('Unsupported chem_params%transport_model; expected 1 (mixture-avg) or 2 (unity Lewis)') end ifThis keeps the behavior explicit and aligns with the project’s error-handling conventions.
Shared setup duplicated between the two models
A large amount of setup (directional
grid_spacing,Ys_L/R/cell, ρ/MW/Rgas/P/Tcomputation) is effectively duplicated in both branches. This has already been called out in a prior review; factoring that shared work into a common block or helper would reduce maintenance risk when new species/thermo fields are added.Since this mirrors an existing high-level suggestion, treat it as a follow-on refactor rather than a blocker.
🧹 Nitpick comments (5)
src/common/include/ExtrusionHardcodedIC.fpp (1)
173-179: 1D IC extrusion now ignoresglobal_offset_x— verify mapping and clean upFor the 1D case you still compute
global_offset_x, but the assignment now usesstored_values(i + 1, 1, f)instead ofstored_values(idx, 1, f). That changes semantics wheneverglobal_offset_x /= 0(e.g., nontrivial domain offsets or multi-rank layouts) and leavesglobal_offset_xas dead state.Please verify with a 1D multi-rank (or offset) case that the intended cells still pick up the correct data; if 1D is always aligned/no-offset, consider either:
- reverting to
stored_values(idx, 1, f)and relying onglobal_offset_x, or- dropping the
global_offset_xcomputation entirely for the 1D branch and documenting that assumption.src/post_process/m_global_parameters.fpp (1)
303-304: Initialize allchem_paramsmembers for deterministic post-processingYou added
chem_paramsand defaultedgamma_method/transport_modelto1, butdiffusionandreactionsremain uninitialized in this module. For consistency with the simulation globals (where they’re set to.false.) and to avoid any future undefined-use if post_process ever reads them, consider also initializing:chem_params%diffusion = .false. chem_params%reactions = .false. chem_params%gamma_method = 1 chem_params%transport_model = 1Also applies to: 420-422
src/pre_process/m_global_parameters.fpp (1)
220-220: Pre-processchem_paramsshould also fully initialize its flagsHere
chem_paramsis introduced and onlygamma_method/transport_modelare defaulted. For consistency with the simulation module and to avoid any accidental use of undefined logicals, consider:chem_params%diffusion = .false. chem_params%reactions = .false. chem_params%gamma_method = 1 chem_params%transport_model = 1This keeps semantics uniform across pre/sim/post stages.
Also applies to: 575-577
src/common/m_derived_types.fpp (1)
435-446: Document the allowed values forchemistry_parameters%transport_modelThe new
transport_modelinteger is only interpreted inm_chemistryas1(mixture-averaged) and2(unity Lewis). It would help maintenance to encode that here, e.g.:integer :: transport_model !< 1: mixture-averaged, 2: unity Lewis numberso callers and input tooling know the valid range.
toolchain/mfc/run/case_dicts.py (1)
356-361: Schema update forchem_params%transport_modelis correct; consider future range checksAdding
chem_params%transport_modelas anINTalongsidegamma_methodin the SIMULATION map is exactly what’s needed for the new chemistry parameter to pass validation.If you later want to harden input, you could extend
ParamTypeto support small integer enums and restrict this field to{1, 2}to catch invalid transport models at parse time.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
src/common/include/ExtrusionHardcodedIC.fpp(1 hunks)src/common/m_chemistry.fpp(3 hunks)src/common/m_derived_types.fpp(1 hunks)src/post_process/m_global_parameters.fpp(2 hunks)src/pre_process/m_global_parameters.fpp(2 hunks)src/simulation/m_global_parameters.fpp(1 hunks)src/simulation/m_mpi_proxy.fpp(1 hunks)toolchain/mfc/run/case_dicts.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{fpp,f90}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{fpp,f90}: Use 2-space indentation; continuation lines align beneath &
Use lower-case keywords and intrinsics (do, end subroutine, etc.)
Name modules with m_ pattern (e.g., m_transport)
Name public subroutines with s_ pattern (e.g., s_compute_flux)
Name public functions with f_ pattern
Keep subroutine size ≤ 500 lines, helper subroutines ≤ 150 lines, functions ≤ 100 lines, files ≤ 1000 lines
Limit routine arguments to ≤ 6; use derived-type params struct if more are needed
Forbid goto statements (except in legacy code), COMMON blocks, and save globals
Every argument must have explicit intent; use dimension/allocatable/pointer as appropriate
Call s_mpi_abort() for errors, never use stop or error stop
**/*.{fpp,f90}: Indent 2 spaces; continuation lines align under&
Use lower-case keywords and intrinsics (do,end subroutine, etc.)
Name modules withm_<feature>prefix (e.g.,m_transport)
Name public subroutines ass_<verb>_<noun>(e.g.,s_compute_flux) and functions asf_<verb>_<noun>
Keep private helpers in the module; avoid nested procedures
Enforce size limits: subroutine ≤ 500 lines, helper ≤ 150, function ≤ 100, module/file ≤ 1000
Limit subroutines to ≤ 6 arguments; otherwise pass a derived-type 'params' struct
Avoidgotostatements (except unavoidable legacy); avoid global state (COMMON,save)
Every variable must haveintent(in|out|inout)specification and appropriatedimension/allocatable/pointer
Uses_mpi_abort(<msg>)for error termination instead ofstop
Use!>style documentation for header comments; follow Doxygen Fortran format with!! @paramand!! @returntags
Useimplicit nonestatement in all modules
Useprivatedeclaration followed by explicitpublicexports in modules
Use derived types with pointers for encapsulation (e.g.,pointer, dimension(:,:,:) => null())
Usepureandelementalattributes for side-effect-free functions; combine them for array ...
Files:
src/common/m_derived_types.fppsrc/post_process/m_global_parameters.fppsrc/common/m_chemistry.fppsrc/simulation/m_mpi_proxy.fppsrc/common/include/ExtrusionHardcodedIC.fppsrc/pre_process/m_global_parameters.fppsrc/simulation/m_global_parameters.fpp
src/**/*.fpp
📄 CodeRabbit inference engine (.cursor/rules/mfc-agent-rules.mdc)
src/**/*.fpp: Use.fppfile extension for Fypp preprocessed files; CMake transpiles them to.f90
Start module files with Fypp include for macros:#:include 'macros.fpp'
Use the fyppASSERTmacro for validating conditions:@:ASSERT(predicate, message)
Use fypp macro@:ALLOCATE(var1, var2)for device-aware allocation instead of standard Fortranallocate
Use fypp macro@:DEALLOCATE(var1, var2)for device-aware deallocation instead of standard Fortrandeallocate
Files:
src/common/m_derived_types.fppsrc/post_process/m_global_parameters.fppsrc/common/m_chemistry.fppsrc/simulation/m_mpi_proxy.fppsrc/common/include/ExtrusionHardcodedIC.fppsrc/pre_process/m_global_parameters.fppsrc/simulation/m_global_parameters.fpp
src/simulation/**/*.{fpp,f90}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
src/simulation/**/*.{fpp,f90}: Wrap tight GPU loops with !$acc parallel loop gang vector default(present) reduction(...); add collapse(n) when safe; declare loop-local variables with private(...)
Allocate large GPU arrays with managed memory or move them into persistent !$acc enter data regions at start-up
Avoid stop/error stop inside GPU device code
Ensure GPU code compiles with Cray ftn, NVIDIA nvfortran, GNU gfortran, and Intel ifx/ifort compilers
src/simulation/**/*.{fpp,f90}: Mark GPU-callable helpers with$:GPU_ROUTINE(function_name='...', parallelism='[seq]')immediately after declaration
Do not use OpenACC or OpenMP directives directly; use Fypp macros fromsrc/common/include/parallel_macros.fppinstead
Wrap tight loops with$:GPU_PARALLEL_FOR(private='[...]', copy='[...]')macro; addcollapse=nfor safe nested loop merging
Declare loop-local variables withprivate='[...]'in GPU parallel loop macros
Allocate large arrays withmanagedor move them into a persistent$:GPU_ENTER_DATA(...)region at start-up
Do not placestoporerror stopinside device code
Files:
src/simulation/m_mpi_proxy.fppsrc/simulation/m_global_parameters.fpp
🧠 Learnings (6)
📚 Learning: 2025-11-24T21:50:46.909Z
Learnt from: CR
Repo: MFlowCode/MFC PR: 0
File: .cursor/rules/mfc-agent-rules.mdc:0-0
Timestamp: 2025-11-24T21:50:46.909Z
Learning: Applies to **/*.{fpp,f90} : Name modules with `m_<feature>` prefix (e.g., `m_transport`)
Applied to files:
src/common/m_derived_types.fppsrc/post_process/m_global_parameters.fpp
📚 Learning: 2025-11-24T21:50:46.909Z
Learnt from: CR
Repo: MFlowCode/MFC PR: 0
File: .cursor/rules/mfc-agent-rules.mdc:0-0
Timestamp: 2025-11-24T21:50:46.909Z
Learning: Applies to **/*.{fpp,f90} : Use `private` declaration followed by explicit `public` exports in modules
Applied to files:
src/common/m_chemistry.fpp
📚 Learning: 2025-11-24T21:50:16.713Z
Learnt from: CR
Repo: MFlowCode/MFC PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-24T21:50:16.713Z
Learning: Applies to src/simulation/**/*.{fpp,f90} : Wrap tight GPU loops with !$acc parallel loop gang vector default(present) reduction(...); add collapse(n) when safe; declare loop-local variables with private(...)
Applied to files:
src/common/m_chemistry.fppsrc/simulation/m_mpi_proxy.fpp
📚 Learning: 2025-11-24T21:50:46.909Z
Learnt from: CR
Repo: MFlowCode/MFC PR: 0
File: .cursor/rules/mfc-agent-rules.mdc:0-0
Timestamp: 2025-11-24T21:50:46.909Z
Learning: Applies to src/simulation/**/*.{fpp,f90} : Wrap tight loops with `$:GPU_PARALLEL_FOR(private='[...]', copy='[...]')` macro; add `collapse=n` for safe nested loop merging
Applied to files:
src/simulation/m_mpi_proxy.fpp
📚 Learning: 2025-11-24T21:50:46.909Z
Learnt from: CR
Repo: MFlowCode/MFC PR: 0
File: .cursor/rules/mfc-agent-rules.mdc:0-0
Timestamp: 2025-11-24T21:50:46.909Z
Learning: Applies to src/simulation/**/*.{fpp,f90} : Do not use OpenACC or OpenMP directives directly; use Fypp macros from `src/common/include/parallel_macros.fpp` instead
Applied to files:
src/simulation/m_mpi_proxy.fpp
📚 Learning: 2025-11-24T21:50:16.713Z
Learnt from: CR
Repo: MFlowCode/MFC PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-24T21:50:16.713Z
Learning: Applies to src/simulation/**/*.{fpp,f90} : Ensure GPU code compiles with Cray ftn, NVIDIA nvfortran, GNU gfortran, and Intel ifx/ifort compilers
Applied to files:
src/simulation/m_mpi_proxy.fpp
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: Github (macos, mpi, no-debug, false)
- GitHub Check: Github (ubuntu, mpi, no-debug, true)
- GitHub Check: Github (macos, mpi, debug, false)
- GitHub Check: Github (ubuntu, no-mpi, single, no-debug, false)
- GitHub Check: Github (ubuntu, mpi, debug, false)
- GitHub Check: Github (ubuntu, mpi, no-debug, false)
- GitHub Check: Github (ubuntu, mpi, debug, true)
- GitHub Check: Coverage Test on CodeCov
- GitHub Check: Code Cleanliness Check
- GitHub Check: Build & Publish
🔇 Additional comments (3)
src/simulation/m_mpi_proxy.fpp (1)
123-131: Chemistry MPI broadcast extension looks consistentBroadcasting
chem_params%gamma_methodandchem_params%transport_modeltogether asMPI_INTEGERunder the existingif (chemistry)guard is consistent with howchem_paramsis defined and how the other chem flags are handled. This should correctly propagate the new transport model across ranks.src/simulation/m_global_parameters.fpp (1)
646-650: Defaultingchem_params%transport_modelto 1 preserves prior behaviorSetting
chem_params%transport_model = 1alongside the existing defaults fordiffusion,reactions, andgamma_methodkeeps mixture-averaged transport as the default and avoids uninitialized use of the new parameter.src/common/m_chemistry.fpp (1)
11-49: New thermochemistry imports and viscosity helper are consistentImporting
get_mixture_viscosity_mixavg/cp/ enthalpy helpers and wrapping the viscosity call incompute_viscosity_and_inversionis straightforward and keeps the inversion logic in one place. The interface (intent(inout)scalars and Ys arrays) matches the rest of this module’s style.
|
CodeAnt AI is running Incremental review Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
|
@DimAdam-01 is this ready to merge? what about all the reviewer comments? |
|
CodeAnt AI is running Incremental review Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
There was a problem hiding this comment.
3 issues found across 4 files (changes from recent commits).
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="examples/1D_multispecies_diffusion/case.py">
<violation number="1" location="examples/1D_multispecies_diffusion/case.py:56">
P2: Incorrect comment: `transport_model: 2` corresponds to Unity Lewis transport, not Mixture-Average. According to m_chemistry.fpp, transport_model=1 is Mixture-Average and transport_model=2 is Unity Lewis.</violation>
</file>
<file name="src/common/include/ExtrusionHardcodedIC.fpp">
<violation number="1" location="src/common/include/ExtrusionHardcodedIC.fpp:51">
P1: Hardcoded user-specific path should not be committed. This path `/Users/dimitriosadam/Desktop/MFC-Adam/examples/` is specific to a developer's local machine and will break functionality for all other users. Consider reverting to the previous placeholder or making this path configurable via a parameter/environment variable.</violation>
</file>
<file name="docs/documentation/case.md">
<violation number="1" location="docs/documentation/case.md:1004">
P3: Typo: "coeffiecients" should be "coefficients".</violation>
</file>
Reply to cubic to teach it or ask questions. Re-run a review with @cubic-dev-ai review this PR
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
src/common/include/ExtrusionHardcodedIC.fpp (1)
176-178: Revert to useidxfor correct MPI subdomain data mapping.This change removes
global_offset_xfrom the 1D indexing, causingstored_valuesto be indexed with the local grid position (i + 1) rather than the global file position (i + 1 + global_offset_x). In MPI parallel runs, this will map file data to incorrect local grid positions. The 2D (line 185) and 3D (line 194) cases correctly retain offset-adjusted indexing, confirming this is an unintended regression.🔎 Proposed fix
idx = i + 1 + global_offset_x do f = 1, sys_size - q_prim_vf(f)%sf(i, 0, 0) = stored_values(i + 1, 1, f) + q_prim_vf(f)%sf(i, 0, 0) = stored_values(idx, 1, f) end do
🧹 Nitpick comments (1)
docs/documentation/case.md (1)
1007-1007: Fix formatting: misplaced backtick.The backtick and colon placement is inconsistent; the value descriptions should be separated by a colon followed by a space before the backtick.
🔎 Proposed fix
-- `chem_params%transport_model` specifies the methodology for calculating diffusion coefficients and other transport properties,`1` for mixture-average, `2` for Unity-Lewis +- `chem_params%transport_model` specifies the methodology for calculating diffusion coefficients and other transport properties: `1` for mixture-average, `2` for Unity-Lewis
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
docs/documentation/case.mdexamples/1D_multispecies_diffusion/case.pysrc/common/include/ExtrusionHardcodedIC.fpptoolchain/mfc/test/cases.py
✅ Files skipped from review due to trivial changes (1)
- toolchain/mfc/test/cases.py
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{fpp,f90}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{fpp,f90}: Use 2-space indentation; continuation lines align beneath &
Use lower-case keywords and intrinsics (do, end subroutine, etc.)
Name modules with m_ pattern (e.g., m_transport)
Name public subroutines with s_ pattern (e.g., s_compute_flux)
Name public functions with f_ pattern
Keep subroutine size ≤ 500 lines, helper subroutines ≤ 150 lines, functions ≤ 100 lines, files ≤ 1000 lines
Limit routine arguments to ≤ 6; use derived-type params struct if more are needed
Forbid goto statements (except in legacy code), COMMON blocks, and save globals
Every argument must have explicit intent; use dimension/allocatable/pointer as appropriate
Call s_mpi_abort() for errors, never use stop or error stop
**/*.{fpp,f90}: Indent 2 spaces; continuation lines align under&
Use lower-case keywords and intrinsics (do,end subroutine, etc.)
Name modules withm_<feature>prefix (e.g.,m_transport)
Name public subroutines ass_<verb>_<noun>(e.g.,s_compute_flux) and functions asf_<verb>_<noun>
Keep private helpers in the module; avoid nested procedures
Enforce size limits: subroutine ≤ 500 lines, helper ≤ 150, function ≤ 100, module/file ≤ 1000
Limit subroutines to ≤ 6 arguments; otherwise pass a derived-type 'params' struct
Avoidgotostatements (except unavoidable legacy); avoid global state (COMMON,save)
Every variable must haveintent(in|out|inout)specification and appropriatedimension/allocatable/pointer
Uses_mpi_abort(<msg>)for error termination instead ofstop
Use!>style documentation for header comments; follow Doxygen Fortran format with!! @paramand!! @returntags
Useimplicit nonestatement in all modules
Useprivatedeclaration followed by explicitpublicexports in modules
Use derived types with pointers for encapsulation (e.g.,pointer, dimension(:,:,:) => null())
Usepureandelementalattributes for side-effect-free functions; combine them for array ...
Files:
src/common/include/ExtrusionHardcodedIC.fpp
src/**/*.fpp
📄 CodeRabbit inference engine (.cursor/rules/mfc-agent-rules.mdc)
src/**/*.fpp: Use.fppfile extension for Fypp preprocessed files; CMake transpiles them to.f90
Start module files with Fypp include for macros:#:include 'macros.fpp'
Use the fyppASSERTmacro for validating conditions:@:ASSERT(predicate, message)
Use fypp macro@:ALLOCATE(var1, var2)for device-aware allocation instead of standard Fortranallocate
Use fypp macro@:DEALLOCATE(var1, var2)for device-aware deallocation instead of standard Fortrandeallocate
Files:
src/common/include/ExtrusionHardcodedIC.fpp
🧠 Learnings (2)
📚 Learning: 2025-11-24T21:50:46.909Z
Learnt from: CR
Repo: MFlowCode/MFC PR: 0
File: .cursor/rules/mfc-agent-rules.mdc:0-0
Timestamp: 2025-11-24T21:50:46.909Z
Learning: Applies to src/simulation/**/*.{fpp,f90} : Do not use OpenACC or OpenMP directives directly; use Fypp macros from `src/common/include/parallel_macros.fpp` instead
Applied to files:
src/common/include/ExtrusionHardcodedIC.fpp
📚 Learning: 2025-11-24T21:50:16.713Z
Learnt from: CR
Repo: MFlowCode/MFC PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-24T21:50:16.713Z
Learning: Applies to src/simulation/**/*.{fpp,f90} : Avoid stop/error stop inside GPU device code
Applied to files:
src/common/include/ExtrusionHardcodedIC.fpp
🧬 Code graph analysis (1)
examples/1D_multispecies_diffusion/case.py (1)
toolchain/mfc/case.py (1)
Case(19-292)
🪛 GitHub Actions: Spell Check
docs/documentation/case.md
[error] 1004-1004: During step './mfc.sh spelling', typo detected: 'coeffiecients' should be 'coefficients'.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (17)
- GitHub Check: Code Cleanliness Check
- GitHub Check: Coverage Test on CodeCov
- GitHub Check: Github (ubuntu, no-mpi, single, no-debug, false)
- GitHub Check: Github (ubuntu, mpi, debug, false)
- GitHub Check: Github (ubuntu, mpi, debug, true)
- GitHub Check: Github (ubuntu, mpi, no-debug, false)
- GitHub Check: Georgia Tech | Phoenix (gpu-omp)
- GitHub Check: Georgia Tech | Phoenix (cpu)
- GitHub Check: Github (macos, mpi, debug, false)
- GitHub Check: Github (ubuntu, mpi, no-debug, true)
- GitHub Check: Oak Ridge | Frontier (gpu-acc)
- GitHub Check: Github (macos, mpi, no-debug, false)
- GitHub Check: Oak Ridge | Frontier (cpu)
- GitHub Check: Oak Ridge | Frontier (gpu-omp)
- GitHub Check: Georgia Tech | Phoenix (gpu-acc)
- GitHub Check: cubic · AI code reviewer
- GitHub Check: Build & Publish
🔇 Additional comments (1)
examples/1D_multispecies_diffusion/case.py (1)
74-76: Verify species mass fraction initialization.All species mass fractions are initialized to 0.0, which may not be physically meaningful for a multispecies diffusion test. Since the patch uses a hardcoded IC (
hcid=182), verify that the hardcoded patch properly initializes the species distribution. Consider adding a comment explaining that the actual species values are set by the hardcoded IC if that's the intended behavior.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
docs/documentation/case.md (1)
1007-1007: Fix formatting: add space after comma before backtick.Line 1007 is missing a space between the comma and the opening backtick. The text currently reads
properties,1which should be `properties, `1(with a space after the comma).🔎 Proposed fix
-- `chem_params%transport_model` specifies the methodology for calculating diffusion coefficients and other transport properties,`1` for mixture-average, `2` for Unity-Lewis +- `chem_params%transport_model` specifies the methodology for calculating diffusion coefficients and other transport properties, `1` for mixture-average, `2` for Unity-Lewis
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
docs/documentation/case.md
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (13)
- GitHub Check: Code Cleanliness Check
- GitHub Check: Coverage Test on CodeCov
- GitHub Check: Github (ubuntu, no-mpi, single, no-debug, false)
- GitHub Check: Github (macos, mpi, no-debug, false)
- GitHub Check: Github (ubuntu, mpi, debug, false)
- GitHub Check: Oak Ridge | Frontier (gpu-omp)
- GitHub Check: Github (ubuntu, mpi, no-debug, true)
- GitHub Check: Georgia Tech | Phoenix (cpu)
- GitHub Check: Github (macos, mpi, debug, false)
- GitHub Check: Github (ubuntu, mpi, no-debug, false)
- GitHub Check: Github (ubuntu, mpi, debug, true)
- GitHub Check: Georgia Tech | Phoenix (gpu-acc)
- GitHub Check: Build & Publish
|
CodeAnt AI is running Incremental review Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
|
@DimAdam-01 close issues/conversations you have fixed... otherwise I assume they remain open. |
|
/improve |
| do eqn = chemxb, chemxe | ||
| Mass_Diffu_Flux(eqn - chemxb + 1) = rho_cell* & | ||
| diffusivity_cell* & | ||
| dYk_dxi(eqn - chemxb + 1) | ||
| end do | ||
| Mass_Diffu_Energy = rho_cell*diffusivity_cell*dh_dxi |
There was a problem hiding this comment.
Suggestion: Invert the sign of the Unity-Lewis diffusion flux calculation for both species and energy. The current implementation incorrectly points with the gradient, which will cause anti-diffusion and instability. [possible issue, importance: 10]
| do eqn = chemxb, chemxe | |
| Mass_Diffu_Flux(eqn - chemxb + 1) = rho_cell* & | |
| diffusivity_cell* & | |
| dYk_dxi(eqn - chemxb + 1) | |
| end do | |
| Mass_Diffu_Energy = rho_cell*diffusivity_cell*dh_dxi | |
| do i = chemxb, chemxe | |
| dYk_dxi(i - chemxb + 1) = (Ys_R(i - chemxb + 1) - & | |
| Ys_L(i - chemxb + 1))/grid_spacing | |
| end do | |
| ... | |
| do eqn = chemxb, chemxe | |
| Mass_Diffu_Flux(eqn - chemxb + 1) = -rho_cell* & | |
| diffusivity_cell* & | |
| dYk_dxi(eqn - chemxb + 1) | |
| end do | |
| Mass_Diffu_Energy = -rho_cell*diffusivity_cell*dh_dxi |
There was a problem hiding this comment.
The sign is correct
There was a problem hiding this comment.
Question:
The sign is correct
Answer:
This is a classic domain-convention pitfall (sign depends on your coordinate definition, face orientation, and how you define “flux” vs “-flux” in the discretization). Qodo Merge can’t reliably infer that from a small diff, so the best mitigation is to explicitly instruct it to be conservative about “flip the sign” recommendations unless there’s hard evidence (tests, comments, clearly inconsistent usage elsewhere).
Add a repo-level instruction for /improve:
[pr_code_suggestions]
extra_instructions = """\
For PDE/numerics/physics code (fluxes, gradients, diffusion, convection):
- Do NOT suggest changing the sign of a term (e.g., flux = -D*grad(phi)) unless the PR provides explicit evidence (unit tests failing, a reference comment/equation in code, or a clear contradiction with the project's established convention in other files).
- If the sign looks suspicious, ask a verification question instead: clarify coordinate direction, face-normal orientation, and whether the stored variable is "flux" or "-flux".
- Avoid marking sign-convention suggestions as high-importance unless there is direct evidence of instability/anti-diffusion in this codebase.
"""If you want to further reduce “speculative” suggestions, you can also tighten output by filtering low-confidence items:
[pr_code_suggestions]
suggestions_score_threshold = 2(Keep this low; setting it too high can hide useful findings.)
Relevant Sources:
Co-authored-by: qodo-code-review[bot] <151058649+qodo-code-review[bot]@users.noreply.github.com>
|
CodeAnt AI is running Incremental review Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
examples/1D_multispecies_diffusion/case.py (2)
6-6: Remove unused import.The
argparsemodule is imported but never used in this script.🔎 Proposed fix
-import argparse
11-12: Clarify or simplify Cantera solution initialization.The script sets specific TPX conditions for
sol_Lbut only useslen(sol_L.Y)to count species. The actual initial conditions come from the hardcoded IC (hcid=182). Consider either:
- Removing the TPX assignment if only the species count is needed, or
- Adding a comment explaining the purpose of these specific conditions
🔎 Simplified alternative
If only the species count is needed:
ctfile = "gri30.yaml" sol_L = ct.Solution(ctfile) -sol_L.TPX = 300, 8000, "O2:2,N2:2,H2O:5" +# sol_L is used only to determine the number of species in the mechanism
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
examples/1D_multispecies_diffusion/case.py
🧰 Additional context used
🧬 Code graph analysis (1)
examples/1D_multispecies_diffusion/case.py (2)
toolchain/mfc/test/cases.py (4)
alter_3d(322-365)alter_num_fluids(224-291)modify_example_case(1040-1057)alter_capillary(122-126)toolchain/mfc/case.py (1)
Case(19-292)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (16)
- GitHub Check: Github (ubuntu, mpi, debug, false)
- GitHub Check: Github (ubuntu, mpi, no-debug, true)
- GitHub Check: Github (ubuntu, no-mpi, single, no-debug, false)
- GitHub Check: Github (ubuntu, mpi, no-debug, false)
- GitHub Check: Github (macos, mpi, no-debug, false)
- GitHub Check: Github (macos, mpi, debug, false)
- GitHub Check: Georgia Tech | Phoenix (gpu-acc)
- GitHub Check: Github (ubuntu, mpi, debug, true)
- GitHub Check: Oak Ridge | Frontier (gpu-omp)
- GitHub Check: Oak Ridge | Frontier (cpu)
- GitHub Check: Georgia Tech | Phoenix (gpu-omp)
- GitHub Check: Oak Ridge | Frontier (gpu-acc)
- GitHub Check: Georgia Tech | Phoenix (cpu)
- GitHub Check: Code Cleanliness Check
- GitHub Check: Coverage Test on CodeCov
- GitHub Check: Build & Publish
🔇 Additional comments (1)
examples/1D_multispecies_diffusion/case.py (1)
56-56: Transport model configuration is correct.The transport_model value and comment are now consistent. Value
2correctly corresponds to Unity-Lewis transport as documented in the source code.Note: This addresses the issues raised in previous review comments.
2 participants
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.
Fixes #(issue) [optional]
Type of change
Added Unity Lewis transport modeling, with mixture-averaged as the default methodology.
Please delete options that are not relevant.
Scope
If you cannot check the above box, please split your PR into multiple PRs that each have a common goal.
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
Test Configuration:
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
Description
Add Unity Lewis transport model as alternative to mixture-averaged
Implement transport model selection via
chem_params%transport_modelparameterAdd new thermochemistry functions for specific heat and enthalpy calculations
Fix bug in ExtrusionHardcodedIC where global offset was incorrectly applied
Extend MPI broadcasting to include transport model parameter
Diagram Walkthrough
File Walkthrough
ExtrusionHardcodedIC.fpp
Fix global offset bug in extrusion ICsrc/common/include/ExtrusionHardcodedIC.fpp
index
stored_values(idx, 1, f)tostored_values(i + 1, 1, f)for 1D case
m_chemistry.fpp
Implement Unity Lewis transport model optionsrc/common/m_chemistry.fpp
get_mixture_specific_heat_cp_massandget_mixture_enthalpy_massfunctionscalculation
dYk_dxi,Cp_L,Cp_R,diffusivity_L/R/cell,hmix_L/R,dh_dxim_derived_types.fpp
Add transport model parameter to chemistry typesrc/common/m_derived_types.fpp
transport_modelinteger field tochemistry_parameterstypem_mpi_proxy.fpp
Broadcast transport model via MPIsrc/simulation/m_mpi_proxy.fpp
transport_modelparameterm_global_parameters.fpp
Initialize chemistry parameters in post-processsrc/post_process/m_global_parameters.fpp
chem_paramsvariable declaration of typechemistry_parameterschem_params%gamma_methodandchem_params%transport_modeltodefault values (1)
m_global_parameters.fpp
Initialize chemistry parameters in pre-processsrc/pre_process/m_global_parameters.fpp
chem_paramsvariable declaration of typechemistry_parameterschem_params%gamma_methodandchem_params%transport_modeltodefault values (1)
m_global_parameters.fpp
Initialize transport model in simulationsrc/simulation/m_global_parameters.fpp
chem_params%transport_modelto 1(mixture-averaged default)
case_dicts.py
Add transport model to case configurationtoolchain/mfc/run/case_dicts.py
transport_modelto integer parameter dictionary for chemistryparameters
CodeAnt-AI Description
Add selectable transport models (mixture-averaged default) and fix extrusion IC indexing
What Changed
Impact
✅ Can select Unity Lewis transport for diffusion✅ Mixture‑averaged transport used by default for existing cases✅ Fewer incorrect extrusion initial conditions💡 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.
Summary by CodeRabbit
New Features
Configuration
Documentation
Examples
✏️ Tip: You can customize this high-level summary in your review settings.