5 Equation model and relaxation #1094
Conversation
Complete version of the SG EoS
This PR comprises changes to m_phase_change module to include the activation of pT- and pTg-relaxation schemes also for the 5-equation model. It also updates m_checker_common to include the new constraints for the phase change, and includes examples for the new feature
|
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 · |
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 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. WalkthroughMade per‑fluid internal‑energy assignment conditional on Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20–30 minutes
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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 |
codeant-ai
bot
added
the
size:XXL
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}
Nitpicks 🔍
|
![codeant-ai[bot]](https://avatars.githubusercontent.com/in/646884?s=60&v=4){kind=small}
|
CodeAnt AI finished reviewing your PR. |
![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 72 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="toolchain/mfc/case_validator.py">
<violation number="1" location="toolchain/mfc/case_validator.py:337">
P2: Error message is misleading: it claims 'model_eqns == 3 (all relax_model)' but the code only allows `relax_model in [1, 4, 5, 6]` for model_eqns=3. Consider updating the message to accurately describe valid combinations, e.g., 'phase change requires model_eqns == 3 with relax_model in [1, 4, 5, 6], or model_eqns == 2 with relax_model == 5 or 6'.</violation>
</file>
Reply to cubic to teach it or ask questions. Re-run a review with @cubic-dev-ai review this 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: 1
♻️ Duplicate comments (7)
tests/413DD555/golden-metadata.txt (1)
1-190: See earlier comment on golden-metadata.txt portability.This file has the same concerns as tests/76C663B7/golden-metadata.txt regarding machine-specific paths (line 29) and CPU scaling MHz variance (line 163: 92%).
tests/D8136392/golden-metadata.txt (1)
1-190: See earlier comment on golden-metadata.txt portability.This file has the same concerns as tests/76C663B7/golden-metadata.txt regarding machine-specific paths (line 29) and CPU scaling MHz variance (line 163: 85%).
tests/E6DBCBD9/golden-metadata.txt (1)
1-190: See earlier comment on golden-metadata.txt portability.This file has the same concerns as tests/76C663B7/golden-metadata.txt regarding machine-specific paths (line 29) and CPU scaling MHz variance (line 163: 92%).
tests/60EE4401/golden-metadata.txt (1)
1-190: See earlier comment on golden-metadata.txt portability.This file has the same concerns as tests/76C663B7/golden-metadata.txt regarding machine-specific paths (line 29) and CPU scaling MHz variance (line 163: 60%).
tests/E0256CBF/golden-metadata.txt (1)
1-190: See earlier comment on golden-metadata.txt portability.This file has the same concerns as tests/76C663B7/golden-metadata.txt regarding machine-specific paths (line 29) and CPU scaling MHz variance (line 163: 86%).
tests/723DDBCF/golden-metadata.txt (1)
1-190: See earlier comment on golden-metadata.txt portability.This file has the same concerns as tests/76C663B7/golden-metadata.txt regarding machine-specific paths (line 29) and CPU scaling MHz variance (line 163: 87%).
tests/C46769FD/golden-metadata.txt (1)
1-190: See earlier comment on golden-metadata.txt portability.This file has the same concerns as tests/76C663B7/golden-metadata.txt regarding machine-specific paths (line 29) and CPU scaling MHz variance (line 163: 93%).
🧹 Nitpick comments (5)
examples/2D_phasechange_bubble_6Eqn/casefile.py (2)
12-12: Remove or document commented code.The commented temperature assignment should either be removed or explained with a comment indicating why it's preserved.
245-250: Simplify arithmetic or explain the scaling factors.Lines 245-250 multiply by large values (500000 or 1000000) then divide by 100, which simplifies to multiplication by 5000 or 10000. This pattern suggests either:
- Unit conversion that should be documented
- Unintentional complexity that should be simplified
🔎 Simplify if no special reason for the intermediate values:
- "patch_icpp(1)%x_centroid": (xe + xb) * 500000 / 100, - "patch_icpp(1)%y_centroid": (ye + yb) * 500000 / 100, - "patch_icpp(1)%z_centroid": (ze + zb) * 500000 / 100, - "patch_icpp(1)%length_x": (xe - xb) * 1000000 / 100, - "patch_icpp(1)%length_y": (ye - yb) * 1000000 / 100, - "patch_icpp(1)%length_z": (ze - zb) * 1000000 / 100, + "patch_icpp(1)%x_centroid": (xe + xb) * 5000.0, + "patch_icpp(1)%y_centroid": (ye + yb) * 5000.0, + "patch_icpp(1)%z_centroid": (ze + zb) * 5000.0, + "patch_icpp(1)%length_x": (xe - xb) * 10000.0, + "patch_icpp(1)%length_y": (ye - yb) * 10000.0, + "patch_icpp(1)%length_z": (ze - zb) * 10000.0,examples/2D_phasechange_bubble_6Eqn/case.py (2)
94-101: Unused computed variables.Variables
ss(shock speed),cfl,Nz, and several speed-of-sound values for patch 2 (c_wl2,c_wv2,c_a2) are computed but never used in the configuration output. Consider removing them to improve clarity, or add a comment explaining if they're kept for documentation/reference purposes.
236-239: Consider clarifying magic number multipliers in patch geometry.The calculations
500000 / 100(= 5000×) and1000000 / 100(= 10000×) create a background patch significantly larger than the computational domain. While this is a valid pattern for ensuring full domain coverage, the magic numbers reduce readability.Consider adding a brief comment or using named constants:
# Background patch extends well beyond domain to ensure full coverage scale_centroid = 5000 # Places centroid far from origin scale_length = 10000 # Creates oversized patchtoolchain/mfc/test/cases.py (1)
726-726: Consider aligning test label format with existing conventions.The label format
"model equation -> {model_eqns}"differs from the pattern used elsewhere in this file (e.g.,"model_eqns=3"on lines 201, 303, 417). For consistency, consider:-f"Phase Change model {relax_model} -> {num_fluids} Fluid(s) -> model equation -> {model_eqns}" +f"Phase Change model {relax_model} -> {num_fluids} Fluid(s) -> model_eqns={model_eqns}"
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (44)
examples/1D_exp_tube_phasechange_5Eqn/case.py(1 hunks)examples/1D_exp_tube_phasechange_6Eqn/case.py(1 hunks)examples/2D_phasechange_bubble_6Eqn/case.py(1 hunks)examples/2D_phasechange_bubble_6Eqn/casefile.py(1 hunks)examples/2D_shocktube_phasechange_6Eqn/case.py(1 hunks)examples/3D_phasechange_bubble_6Eqn/case.py(1 hunks)src/common/m_phase_change.fpp(1 hunks)tests/02BD9B5E/golden-metadata.txt(1 hunks)tests/18431ACB/golden-metadata.txt(0 hunks)tests/1B300F28/golden-metadata.txt(0 hunks)tests/2486442D/golden-metadata.txt(1 hunks)tests/2C9844EF/golden-metadata.txt(0 hunks)tests/344FAA70/golden-metadata.txt(1 hunks)tests/413DD555/golden-metadata.txt(1 hunks)tests/60EE4401/golden-metadata.txt(1 hunks)tests/610959C9/golden-metadata.txt(0 hunks)tests/623B6C30/golden-metadata.txt(1 hunks)tests/67C777D8/golden-metadata.txt(0 hunks)tests/6F296065/golden-metadata.txt(0 hunks)tests/723DDBCF/golden-metadata.txt(1 hunks)tests/72FD6FF5/golden-metadata.txt(0 hunks)tests/76C663B7/golden-metadata.txt(1 hunks)tests/786DE444/golden-metadata.txt(0 hunks)tests/82A64F89/golden-metadata.txt(0 hunks)tests/842C6FFC/golden-metadata.txt(0 hunks)tests/8D0E7E1D/golden-metadata.txt(1 hunks)tests/8D6E051E/golden-metadata.txt(1 hunks)tests/9EF19F0A/golden-metadata.txt(0 hunks)tests/A83CADB5/golden-metadata.txt(0 hunks)tests/AAE5C27B/golden-metadata.txt(1 hunks)tests/B2EFB9F7/golden-metadata.txt(1 hunks)tests/B97D4966/golden-metadata.txt(0 hunks)tests/C46769FD/golden-metadata.txt(1 hunks)tests/D21F4F38/golden-metadata.txt(1 hunks)tests/D3C860B9/golden-metadata.txt(0 hunks)tests/D8136392/golden-metadata.txt(1 hunks)tests/DA8AF07E/golden-metadata.txt(0 hunks)tests/E0256CBF/golden-metadata.txt(1 hunks)tests/E6DBCBD9/golden-metadata.txt(1 hunks)tests/F4D00494/golden-metadata.txt(1 hunks)tests/FA4D8FEF/golden-metadata.txt(0 hunks)tests/FA4D8FEF/golden.txt(0 hunks)toolchain/mfc/case_validator.py(1 hunks)toolchain/mfc/test/cases.py(1 hunks)
💤 Files with no reviewable changes (17)
- tests/9EF19F0A/golden-metadata.txt
- tests/842C6FFC/golden-metadata.txt
- tests/DA8AF07E/golden-metadata.txt
- tests/FA4D8FEF/golden-metadata.txt
- tests/6F296065/golden-metadata.txt
- tests/A83CADB5/golden-metadata.txt
- tests/1B300F28/golden-metadata.txt
- tests/610959C9/golden-metadata.txt
- tests/18431ACB/golden-metadata.txt
- tests/D3C860B9/golden-metadata.txt
- tests/786DE444/golden-metadata.txt
- tests/B97D4966/golden-metadata.txt
- tests/72FD6FF5/golden-metadata.txt
- tests/FA4D8FEF/golden.txt
- tests/67C777D8/golden-metadata.txt
- tests/82A64F89/golden-metadata.txt
- tests/2C9844EF/golden-metadata.txt
🧰 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/m_phase_change.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_phase_change.fpp
🧠 Learnings (4)
📚 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: Add and update focused tests for changes; ensure no regressions in golden test outputs without clear justification
Applied to files:
tests/723DDBCF/golden-metadata.txttests/2486442D/golden-metadata.txttests/8D0E7E1D/golden-metadata.txttests/C46769FD/golden-metadata.txttests/76C663B7/golden-metadata.txttests/E0256CBF/golden-metadata.txttests/D21F4F38/golden-metadata.txttests/D8136392/golden-metadata.txttests/AAE5C27B/golden-metadata.txttests/344FAA70/golden-metadata.txttests/60EE4401/golden-metadata.txttests/02BD9B5E/golden-metadata.txttests/F4D00494/golden-metadata.txttests/413DD555/golden-metadata.txttests/B2EFB9F7/golden-metadata.txttests/623B6C30/golden-metadata.txttests/E6DBCBD9/golden-metadata.txttests/8D6E051E/golden-metadata.txt
📚 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: Draft a step-by-step plan before making changes; build after each step using `./mfc.sh build -t pre_process simulation -j $(nproc)`
Applied to files:
tests/723DDBCF/golden-metadata.txttests/2486442D/golden-metadata.txttests/8D0E7E1D/golden-metadata.txttests/C46769FD/golden-metadata.txttests/76C663B7/golden-metadata.txttests/E0256CBF/golden-metadata.txttests/D21F4F38/golden-metadata.txttests/D8136392/golden-metadata.txttests/AAE5C27B/golden-metadata.txttests/344FAA70/golden-metadata.txttests/60EE4401/golden-metadata.txttests/02BD9B5E/golden-metadata.txttests/F4D00494/golden-metadata.txttests/413DD555/golden-metadata.txttests/B2EFB9F7/golden-metadata.txttests/623B6C30/golden-metadata.txttests/E6DBCBD9/golden-metadata.txttests/8D6E051E/golden-metadata.txt
📚 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:
tests/723DDBCF/golden-metadata.txttests/2486442D/golden-metadata.txttests/8D0E7E1D/golden-metadata.txttests/C46769FD/golden-metadata.txttests/76C663B7/golden-metadata.txttests/E0256CBF/golden-metadata.txttests/D21F4F38/golden-metadata.txttests/D8136392/golden-metadata.txttests/AAE5C27B/golden-metadata.txttests/344FAA70/golden-metadata.txttests/60EE4401/golden-metadata.txttests/02BD9B5E/golden-metadata.txttests/F4D00494/golden-metadata.txttests/413DD555/golden-metadata.txttests/B2EFB9F7/golden-metadata.txttests/623B6C30/golden-metadata.txttests/E6DBCBD9/golden-metadata.txttests/8D6E051E/golden-metadata.txt
📚 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: Compile with Cray `ftn` or NVIDIA `nvfortran` for GPU offloading; also build CPU-only with GNU `gfortran` and Intel `ifx`/`ifort` for portability
Applied to files:
tests/723DDBCF/golden-metadata.txttests/2486442D/golden-metadata.txttests/8D0E7E1D/golden-metadata.txttests/C46769FD/golden-metadata.txttests/76C663B7/golden-metadata.txttests/E0256CBF/golden-metadata.txttests/D21F4F38/golden-metadata.txttests/D8136392/golden-metadata.txttests/AAE5C27B/golden-metadata.txttests/344FAA70/golden-metadata.txttests/60EE4401/golden-metadata.txttests/02BD9B5E/golden-metadata.txttests/F4D00494/golden-metadata.txttests/413DD555/golden-metadata.txttests/B2EFB9F7/golden-metadata.txttests/623B6C30/golden-metadata.txttests/E6DBCBD9/golden-metadata.txttests/8D6E051E/golden-metadata.txt
🧬 Code graph analysis (2)
examples/2D_shocktube_phasechange_6Eqn/case.py (1)
toolchain/mfc/case.py (1)
Case(19-292)
toolchain/mfc/test/cases.py (1)
toolchain/mfc/test/case.py (3)
push(317-319)define_case_d(331-347)pop(321-322)
🪛 GitHub Actions: Lint Toolchain
toolchain/mfc/case_validator.py
[error] 333-333: R1714: Consider merging these comparisons with 'in' by using 'relax_model in (5, 6)'. Use a set instead if elements are hashable. (consider-using-in)
⏰ 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: 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: Oak Ridge | Frontier (gpu-omp)
- GitHub Check: Github (macos, mpi, no-debug, false)
- GitHub Check: Georgia Tech | Phoenix (gpu-omp)
- GitHub Check: Github (macos, mpi, debug, false)
- GitHub Check: Github (ubuntu, mpi, debug, true)
- GitHub Check: Github (ubuntu, mpi, no-debug, true)
- GitHub Check: Georgia Tech | Phoenix (gpu-acc)
- GitHub Check: Code Cleanliness Check
- GitHub Check: Coverage Test on CodeCov
- GitHub Check: Build & Publish
🔇 Additional comments (12)
tests/D21F4F38/golden-metadata.txt (1)
29-29: Golden-metadata.txt is informational only and not used in test comparisons.The
golden-metadata.txtfile is generated as a side effect of saving the test's golden pack file and documents the system configuration (paths, hostname, git status, CPU info) used when the test was generated. It is never loaded or compared during testing. Test comparisons only usegolden.txt, which contains the numerical data from simulation outputs. The absolute paths and system-specific values in metadata are expected and do not affect test portability or results across different machines or CI environments.tests/76C663B7/golden-metadata.txt (1)
1-190: Golden-metadata.txt files are informational only and not used in regression testing.The golden-metadata.txt files captured here are metadata/logging files created by the syscheck utility during test execution, not regression test reference files. The actual regression tests compare
golden.txtfiles (which contain computational results), notgolden-metadata.txtfiles (which record system configuration).Because golden-metadata.txt is not used in test comparisons, the concerns about hard-coded paths (line 29) and CPU scaling MHz variance (line 163) do not affect test reliability. These files are appropriately version-controlled as informational records of the build and execution environment.
src/common/m_phase_change.fpp (1)
263-265: The conditional correctly guards internal energy field assignment to model_eqns == 3.The code is safe and requires no changes. The assignment to
q_cons_vf(i + intxb - 1)%sf(j, k, l)only occurs whenmodel_eqns == 3, and at that point,internalEnergies_idx(and thusintxb) is explicitly initialized inm_global_parameters.fpp(lines 773-774). For other model types (model_eqns== 1, 2, or 4),internalEnergies_idxis not set, but the conditional prevents the assignment from executing. Internal energy fields are neither used nor required for non-6-equation models—they are model-specific state variables present only in the 6-equation system.Likely an incorrect or invalid review comment.
tests/623B6C30/golden-metadata.txt (1)
1-190: LGTM - Golden metadata artifact for new test case.This test metadata file captures the build environment and hardware configuration for test validation purposes.
tests/AAE5C27B/golden-metadata.txt (1)
1-190: LGTM - Golden metadata artifact for test case AAE5C27B.Correctly captures the test environment configuration for regression validation.
examples/2D_shocktube_phasechange_6Eqn/case.py (1)
1-267: Well-structured 2D shock-tube configuration for 6-equation phase-change model.The script correctly:
- Defines three-fluid system (liquid water, vapor, air) with appropriate thermodynamic properties
- Computes shock relations and derives simulation parameters
- Configures three patches with proper geometry and state variables
- Uses
model_eqns=3withrelax_model=6, which is a valid combination per the updated validatorThe physical setup and numerical parameters are well-documented.
examples/1D_exp_tube_phasechange_6Eqn/case.py (1)
1-193: LGTM - 1D expansion tube configuration for 6-equation phase-change model.The script correctly sets up an expansion tube scenario with:
- Three-fluid system with appropriate thermodynamic properties
- Opposing patch velocities (
-veland+vel) to generate expansion waves- Valid
model_eqns=3withrelax_model=6configurationexamples/1D_exp_tube_phasechange_5Eqn/case.py (1)
1-193: LGTM - 1D expansion tube configuration for 5-equation phase-change model.This example demonstrates the new feature introduced by this PR: phase-change relaxation with
model_eqns=2(5-equation model) andrelax_model=6. The configuration mirrors the 6-equation version, enabling direct comparison between the two models as shown in the PR's test results.examples/2D_phasechange_bubble_6Eqn/case.py (1)
175-282: Configuration structure looks correct.The simulation configuration properly defines:
- 6-equation model (
model_eqns: 3,relax_model: 6)- Three fluids with complete thermodynamic properties
- Two patches with proper volume fractions summing appropriately
- Cylindrical coordinates enabled for axisymmetric 2D simulation
toolchain/mfc/test/cases.py (1)
725-786: Stack push/pop balance is correct for the new nested loop structure.The refactored phase-change test generation properly maintains stack balance:
- For
num_fluids == 3: 3 pushes (phase change, fluid 3, velocity) → 3 pops- For
num_fluids != 3: 2 pushes (phase change, velocity) → 2 popsThe new structure correctly generates distinct test cases for each
model_eqnsvalue (3 and 2), with appropriate labeling.examples/3D_phasechange_bubble_6Eqn/case.py (1)
1-296: 3D configuration correctly extends the 2D pattern.The script properly adapts the 2D phase-change bubble case for 3D:
- Uses appropriate 3D geometries (cuboid=9, sphere=8)
- Includes z-domain parameters and stretch settings
- Defines all three velocity components
- Cartesian coordinates (
cyl_coord: "F") appropriate for 3DThe same minor suggestions from the 2D review apply here (unused variables like
ss,cfl, and magic number multipliers in patch geometry).tests/2486442D/golden-metadata.txt (1)
1-189: The machine-specific data in golden-metadata.txt does not affect test portability. The metadata file is written separately from the pack data and is used for reference/debugging only—it is never loaded or compared during test validation. Test comparison occurs only on the numerical pack data (golden.txt) using tolerance-based floating-point comparison, which is machine-independent.Likely an incorrect or invalid review comment.
Co-authored-by: codeant-ai[bot] <151821869+codeant-ai[bot]@users.noreply.github.com>
Co-authored-by: codeant-ai[bot] <151821869+codeant-ai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
examples/3D_phasechange_bubble_6Eqn/case.py (1)
169-169: Off-by-one: Missing +1 for initial save state.The comment on line 147 explicitly states the save count should account for the initial state: "save frequency = SF + 1". Computing
AS = int(Nt // SF)omits the initial save (at t=0) and produces one fewer save entry than documented.🔎 Apply this diff to match the documented behavior:
-AS = int(Nt // SF) +AS = int(Nt // SF + 1)
🧹 Nitpick comments (1)
examples/3D_phasechange_bubble_6Eqn/case.py (1)
12-12: Consider removing the commented-out debug value.The commented temperature value of 7.914 K appears to be a leftover from debugging or experimentation and can be removed for cleaner code.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
examples/3D_phasechange_bubble_6Eqn/case.py(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: CR
Repo: MFlowCode/MFC PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-24T21:50:16.713Z
Learning: Add and update focused tests for changes; ensure no regressions in golden test outputs without clear justification
🧬 Code graph analysis (1)
examples/3D_phasechange_bubble_6Eqn/case.py (1)
toolchain/mfc/test/cases.py (7)
alter_viscosity(787-838)alter_phasechange(719-785)alter_bubbles(511-588)alter_num_fluids(224-291)alter_3d(322-365)alter_body_forces(654-684)chemistry_cases(1061-1097)
⏰ 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: Coverage Test on CodeCov
- GitHub Check: Code Cleanliness Check
- GitHub Check: Oak Ridge | Frontier (gpu-acc)
- GitHub Check: Github (ubuntu, no-mpi, single, no-debug, false)
- GitHub Check: Github (macos, mpi, no-debug, false)
- GitHub Check: Github (macos, mpi, debug, false)
- GitHub Check: Github (ubuntu, mpi, debug, true)
- GitHub Check: Github (ubuntu, mpi, debug, false)
- GitHub Check: Github (ubuntu, mpi, no-debug, false)
- GitHub Check: Github (ubuntu, mpi, no-debug, true)
- GitHub Check: Georgia Tech | Phoenix (cpu)
- GitHub Check: Georgia Tech | Phoenix (gpu-acc)
- GitHub Check: Oak Ridge | Frontier (cpu)
- GitHub Check: Georgia Tech | Phoenix (gpu-omp)
- GitHub Check: Oak Ridge | Frontier (gpu-omp)
- GitHub Check: Build & Publish
🔇 Additional comments (5)
examples/3D_phasechange_bubble_6Eqn/case.py (5)
14-93: LGTM: Fluid property definitions are physically sound.The fluid properties for liquid water (stiffened gas), water vapor, and air (ideal gases) are correctly computed using appropriate equations of state and thermodynamic relations.
103-115: LGTM: Volume fraction calculations are algebraically correct.The volume fractions for both patches are properly computed, with patch 1 being nearly pure liquid water and patch 2 containing a vapor-air mixture with concentration C0=0.25.
165-165: Verified: Time step calculation is correct.The use of
math.ceil()ensures sufficient time steps are allocated. This appears to address the concern raised in previous reviews about floor-division.
174-296: LGTM: JSON configuration structure is comprehensive and consistent.The simulation configuration properly specifies:
- 6-equation model (model_eqns=3) with pTg-relaxation (relax_model=6)
- Three-fluid system with correct thermodynamic properties
- Appropriate solver settings (WENO order 3, Riemann solver 2)
- Two patches: background cuboid and spherical bubble
All fluid properties in the JSON match the computed values from earlier in the file (aside from the critical geometry issue flagged separately).
98-98: Shock Mach number formula for stiffened gas is correct.The formula at line 98 correctly implements the modified shock relation for stiffened gas EOS. The factor
(p02 / (p02 + pia))properly accounts for the stiffening pressure, and the calculation is consistent with the stiffened gas form used throughout for density, sound speed, and other thermodynamic quantities. The formula reduces to the standard ideal gas relation whenpia = 0.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
examples/2D_shocktube_phasechange_6Eqn/case.py (1)
145-146: Optional: Redundantint()calls.The past review comment about using
math.ceil()has been addressed. In Python 3,math.ceil()already returns anint, so the outerint()calls are redundant (though harmless).🔎 Optional simplification:
-NtA = int(math.ceil(tendA / dt)) -AS = int(math.ceil(NtA / SF)) +NtA = math.ceil(tendA / dt) +AS = math.ceil(NtA / SF)
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
examples/2D_shocktube_phasechange_6Eqn/case.py(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: CR
Repo: MFlowCode/MFC PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-24T21:50:16.713Z
Learning: Add and update focused tests for changes; ensure no regressions in golden test outputs without clear justification
🧬 Code graph analysis (1)
examples/2D_shocktube_phasechange_6Eqn/case.py (1)
toolchain/mfc/test/cases.py (9)
alter_hypoelasticity(590-652)alter_phasechange(719-785)alter_viscosity(787-838)alter_bubbles(511-588)alter_2d(293-320)alter_mixlayer_perturb(686-717)alter_3d(322-365)chemistry_cases(1061-1097)alter_riemann_solvers(190-209)
🪛 Ruff (0.14.8)
examples/2D_shocktube_phasechange_6Eqn/case.py
145-145: Value being cast to int is already an integer
Remove unnecessary int call
(RUF046)
146-146: Value being cast to int is already an integer
Remove unnecessary int call
(RUF046)
⏰ 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: Coverage Test on CodeCov
- GitHub Check: Code Cleanliness Check
- 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: Oak Ridge | Frontier (gpu-omp)
- GitHub Check: Github (ubuntu, mpi, no-debug, true)
- GitHub Check: Github (macos, mpi, debug, false)
- GitHub Check: Github (ubuntu, mpi, debug, false)
- GitHub Check: Georgia Tech | Phoenix (gpu-omp)
- GitHub Check: Github (ubuntu, mpi, debug, true)
- GitHub Check: Georgia Tech | Phoenix (cpu)
- GitHub Check: Build & Publish
🔇 Additional comments (2)
examples/2D_shocktube_phasechange_6Eqn/case.py (2)
1-107: LGTM: Physical property setup is correct.The fluid property definitions (liquid water, vapor, air) and derived quantities (gamma, speed of sound, shock relations) follow standard thermodynamic formulas. The three-fluid volume fraction constraint is properly enforced.
246-260: LGTM: Consistent fluid property transformations.The fluid property definitions apply a consistent transformation pattern to gamma and pi_inf for all three fluids, which aligns with the framework's internal representation.
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.
8 issues found across 123 files (changes from recent commits).
Note: This PR contains a large number of files. cubic only reviews up to 75 files per PR, so some files may not have been reviewed.
Prompt for AI agents (all 8 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">
P2: Spurious `.` characters in the markdown table will break rendering. There's a standalone `.` after the heading and multiple table cells have errant `.` characters appended (e.g., `| Parameter . |`). These should be removed for the table to render correctly.</violation>
</file>
<file name="docs/documentation/docker.md">
<violation number="1">
P2: Broken link: `examples.md` does not exist in `docs/documentation/`. The directory contains `case.md`, `getting-started.md`, `running.md`, etc., but no `examples.md`. Consider using the correct relative path or keeping the original absolute URL.</violation>
</file>
<file name="CMakeLists.txt">
<violation number="1">
P2: Missing dependency on `gen_constraints.sh` script. The custom command runs this script but doesn't list it in DEPENDS, so modifications to the script won't trigger regeneration of `case_constraints.md`. Compare with the similar `examples.md` command which correctly includes its shell script in DEPENDS.</violation>
</file>
<file name="examples/3D_lagrange_bubblescreen/case.py">
<violation number="1">
P1: Gas constants appear to be swapped. `bub_pp%R_v` should use `MW_v` in the denominator, not `MW_g`.</violation>
<violation number="2">
P1: Specific heats appear to be swapped. `bub_pp%cp_v` should use `cp_v`, not `cp_g`.</violation>
<violation number="3">
P1: Thermal conductivities appear to be swapped. `bub_pp%k_v` should use `k_v`, not `k_g`.</violation>
<violation number="4">
P1: Molar weights appear to be swapped. `bub_pp%M_v` should receive `MW_v` (18.0 for water vapor), not `MW_g` (28.0 for gas).</violation>
<violation number="5">
P1: Vapor and gas specific heat ratios appear to be swapped. `bub_pp%gam_v` should receive `gam_v` (1.333 for vapor), not `gam_g` (1.4 for gas). This will cause incorrect thermodynamic calculations.</violation>
</file>
Reply to cubic to teach it or ask questions. Re-run a review with @cubic-dev-ai review this PR
Into single parametrized scripts, in which the model equation can be chosen
There was a problem hiding this comment.
4 issues found across 11 files (changes from recent commits).
Prompt for AI agents (all 4 issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="examples/2D_phasechange_bubble/casefile.py">
<violation number="1" location="examples/2D_phasechange_bubble/casefile.py:11">
P1: Missing `default` value or `required=True` for `--model_eqns` argument. If the script is run without this argument, `args.model_eqns` will be `None`, resulting in invalid JSON output. Consider adding `default=3` to maintain backward compatibility with the original hardcoded value, or add `required=True` if the argument must always be explicitly provided.</violation>
</file>
<file name="examples/2D_phasechange_bubble/case.py">
<violation number="1" location="examples/2D_phasechange_bubble/case.py:11">
P1: Missing default value for `--model_eqns` argument. If not provided, `args.model_eqns` will be `None`, causing the simulation configuration to have an invalid `model_eqns` value. Consider adding `default=3` to maintain backward compatibility with the original hardcoded value, or set `required=True` to enforce explicit selection.</violation>
</file>
<file name="examples/2D_shocktube_phasechange/case.py">
<violation number="1" location="examples/2D_shocktube_phasechange/case.py:11">
P2: The `--model_eqns` argument has no default value and isn't marked as required. If run without this flag, `args.model_eqns` will be `None`, causing invalid configuration. Consider adding `default=3` or `required=True`.</violation>
</file>
<file name="examples/3D_phasechange_bubble/case.py">
<violation number="1" location="examples/3D_phasechange_bubble/case.py:11">
P1: Missing default value for `--model_eqns` argument. If the script is run without this argument, `args.model_eqns` will be `None`, causing an invalid configuration. Consider adding `default=3` to maintain backward compatibility with the original hardcoded value.</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: 1
♻️ Duplicate comments (4)
examples/3D_phasechange_bubble/case.py (1)
2-13: Duplicate: Same CLI setup issues as 2D_phasechange_bubble/case.py.This argparse block is identical to the one in examples/2D_phasechange_bubble/case.py (lines 2-13). All concerns raised there apply here:
- Missing
required=Trueon--model_eqns(critical)- Code duplication across 5 files (essential refactor needed)
- Unused
--mfcargument (verify intent)- Confusing help text for model_eqns
examples/2D_phasechange_bubble/casefile.py (1)
2-13: Duplicate: Same CLI setup issues as other phase-change examples.This argparse block is duplicated across multiple files. See the detailed comment on examples/2D_phasechange_bubble/case.py lines 2-13 for all concerns:
- Missing
required=Trueon--model_eqns(critical)- Code duplication (essential refactor needed)
- Unused
--mfcargument- Confusing help text
examples/2D_shocktube_phasechange/case.py (1)
2-13: Duplicate: Same CLI setup issues as other phase-change examples.This argparse block is duplicated across multiple files. See the detailed comment on examples/2D_phasechange_bubble/case.py lines 2-13 for all concerns:
- Missing
required=Trueon--model_eqns(critical)- Code duplication (essential refactor needed)
- Unused
--mfcargument- Confusing help text
examples/1D_exp_tube_phasechange/case.py (1)
2-13: Duplicate: Same CLI setup issues as other phase-change examples.This argparse block is duplicated across multiple files. See the detailed comment on examples/2D_phasechange_bubble/case.py lines 2-13 for all concerns:
- Missing
required=Trueon--model_eqns(critical)- Code duplication (essential refactor needed)
- Unused
--mfcargument- Confusing help text
Additionally, verify that the test framework in toolchain/mfc/test/cases.py (which shows
'model_eqns': 3hardcoded in the alter_phasechange function around line 725) is updated to test both model_eqns=2 and model_eqns=3 scenarios, aligning with the PR's goal to support both 5- and 6-equation models.Run the following script to check if test case generation covers both model_eqns values:
#!/bin/bash # Check if alter_phasechange iterates over model_eqns values ast-grep --pattern $'def alter_phasechange($$$) { $$$ for $_ in $$$: $$$ model_eqns $$$ }'
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
examples/1D_exp_tube_phasechange/case.py(2 hunks)examples/2D_phasechange_bubble/case.py(2 hunks)examples/2D_phasechange_bubble/casefile.py(2 hunks)examples/2D_shocktube_phasechange/case.py(2 hunks)examples/3D_phasechange_bubble/case.py(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
examples/1D_exp_tube_phasechange/case.py (1)
toolchain/mfc/test/cases.py (1)
alter_phasechange(719-785)
examples/3D_phasechange_bubble/case.py (2)
toolchain/mfc/args.py (1)
parse(10-193)toolchain/mfc/state.py (1)
MFCConfig(11-79)
⏰ 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). (11)
- GitHub Check: cubic · AI code reviewer
- GitHub Check: Github (ubuntu, mpi, no-debug, true)
- GitHub Check: Github (macos, mpi, no-debug, false)
- GitHub Check: Github (macos, mpi, debug, false)
- GitHub Check: Georgia Tech | Phoenix (gpu-acc)
- GitHub Check: Oak Ridge | Frontier (gpu-omp)
- GitHub Check: Oak Ridge | Frontier (gpu-acc)
- GitHub Check: Oak Ridge | Frontier (cpu)
- GitHub Check: Georgia Tech | Phoenix (gpu-omp)
- GitHub Check: Georgia Tech | Phoenix (cpu)
- GitHub Check: Build & Publish
🔇 Additional comments (5)
examples/2D_phasechange_bubble/case.py (1)
216-216: Verify that model_eqns is never None.The dynamic model selection via
args.model_eqnscorrectly enables runtime configuration. However, ensure that the argument is always provided (see comment on lines 2-13 about making it required) to preventnullvalues in the JSON output.examples/3D_phasechange_bubble/case.py (1)
222-222: LGTM with caveat.Dynamic model selection correctly implemented. Ensure
--model_eqnsis made required to prevent null values.examples/2D_phasechange_bubble/casefile.py (1)
223-223: LGTM with caveat.Dynamic model selection correctly implemented. Ensure
--model_eqnsis made required to prevent null values.examples/2D_shocktube_phasechange/case.py (1)
187-187: LGTM with caveat.Dynamic model selection correctly implemented. Ensure
--model_eqnsis made required to prevent null values.examples/1D_exp_tube_phasechange/case.py (1)
137-137: LGTM with caveat.Dynamic model selection correctly implemented. Ensure
--model_eqnsis made required to prevent null values.
adjusting indentation so the code does not crash
Co-authored-by: cubic-dev-ai[bot] <191113872+cubic-dev-ai[bot]@users.noreply.github.com>
Co-authored-by: cubic-dev-ai[bot] <191113872+cubic-dev-ai[bot]@users.noreply.github.com>
Co-authored-by: cubic-dev-ai[bot] <191113872+cubic-dev-ai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
examples/2D_phasechange_bubble/case.py(2 hunks)examples/2D_phasechange_bubble/casefile.py(3 hunks)examples/2D_shocktube_phasechange/case.py(2 hunks)examples/3D_phasechange_bubble/case.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- examples/2D_phasechange_bubble/casefile.py
- examples/2D_phasechange_bubble/case.py
- examples/2D_shocktube_phasechange/case.py
🧰 Additional context used
🪛 GitHub Actions: Lint Toolchain
examples/3D_phasechange_bubble/case.py
[error] 8-8: Trailing whitespace (trailing-whitespace) detected by pylint during './mfc.sh lint'.
⏰ 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). (1)
- GitHub Check: Github (ubuntu, no-mpi, single, no-debug, false)
🔇 Additional comments (2)
examples/3D_phasechange_bubble/case.py (2)
9-12: LGTM! CLI argument design is solid.The CLI arguments are well-designed:
--mfcproperly defaults to an empty dict via JSON parsing--model_eqnsnow includesdefault=3(addressing the previous review comment), maintaining backward compatibility with the original hardcoded value- Choices constraint
[2, 3]aligns with the 5-equation and 6-equation models supported by the PR
222-222: LGTM! Configuration now uses CLI argument.The change from a hardcoded
"model_eqns": 3to"model_eqns": args.model_eqnssuccessfully makes the model selection configurable while maintaining the original default behavior.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
examples/3D_phasechange_bubble/case.py(3 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: CR
Repo: MFlowCode/MFC PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-24T21:50:16.713Z
Learning: Add and update focused tests for changes; ensure no regressions in golden test outputs without clear justification
🧬 Code graph analysis (1)
examples/3D_phasechange_bubble/case.py (1)
toolchain/mfc/args.py (2)
parse(10-193)add_common_arguments(50-77)
🪛 GitHub Actions: Lint Toolchain
examples/3D_phasechange_bubble/case.py
[warning] 7-7: Trailing whitespace (trailing-whitespace)
[warning] 306-306: Trailing newlines (trailing-newlines)
⏰ 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: Georgia Tech | Phoenix (gpu-omp)
- GitHub Check: Github (ubuntu, no-mpi, single, no-debug, false)
- GitHub Check: Github (ubuntu, mpi, debug, true)
- GitHub Check: Georgia Tech | Phoenix (gpu-acc)
- GitHub Check: Oak Ridge | Frontier (gpu-acc)
- GitHub Check: Github (macos, mpi, debug, false)
- GitHub Check: Georgia Tech | Phoenix (cpu)
- GitHub Check: Github (macos, mpi, no-debug, false)
- GitHub Check: Oak Ridge | Frontier (cpu)
- GitHub Check: Oak Ridge | Frontier (gpu-omp)
- GitHub Check: Build & Publish
🔇 Additional comments (2)
examples/3D_phasechange_bubble/case.py (2)
2-12: LGTM! CLI argument parsing correctly implemented.The argument parser setup is well-structured with appropriate defaults and constraints. The
default=3maintains backward compatibility with the original hardcoded value, andchoices=[2, 3]properly restricts model selection.Note: Line 7 has trailing whitespace flagged by the linter.
221-221: LGTM! Dynamic model selection correctly wired.The replacement of the hardcoded value with
args.model_eqnsproperly enables runtime model selection between 5-equation and 6-equation models, aligning with the PR's objectives.
|
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: 2
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
examples/1D_exp_tube_phasechange/case.py(2 hunks)examples/2D_phasechange_bubble/case.py(3 hunks)examples/2D_phasechange_bubble/casefile.py(3 hunks)examples/2D_shocktube_phasechange/case.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- examples/2D_shocktube_phasechange/case.py
- examples/2D_phasechange_bubble/case.py
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: CR
Repo: MFlowCode/MFC PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-24T21:50:16.713Z
Learning: Add and update focused tests for changes; ensure no regressions in golden test outputs without clear justification
🧬 Code graph analysis (1)
examples/2D_phasechange_bubble/casefile.py (1)
toolchain/mfc/args.py (2)
parse(10-193)add_common_arguments(50-77)
🪛 GitHub Actions: Lint Toolchain
examples/1D_exp_tube_phasechange/case.py
[warning] 8-8: Pylint: Trailing whitespace (trailing-whitespace) detected (C0303).
⏰ 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: Coverage Test on CodeCov
- GitHub Check: Code Cleanliness Check
- GitHub Check: Github (macos, mpi, no-debug, false)
- GitHub Check: Georgia Tech | Phoenix (cpu)
- GitHub Check: Github (ubuntu, mpi, no-debug, false)
- GitHub Check: Github (ubuntu, mpi, debug, true)
- GitHub Check: Oak Ridge | Frontier (gpu-omp)
- GitHub Check: Github (macos, mpi, debug, false)
- 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: Georgia Tech | Phoenix (gpu-omp)
- GitHub Check: Build & Publish
🔇 Additional comments (2)
examples/2D_phasechange_bubble/casefile.py (1)
223-223: LGTM: Runtime model selection integrated correctly.The configuration now uses
args.model_eqnsfrom the CLI, enabling dynamic switching between 5-equation and 6-equation models as intended by this PR.examples/1D_exp_tube_phasechange/case.py (1)
137-137: LGTM: Consistent with other examples.The use of
args.model_eqnsenables runtime model selection, and the implementation is consistent with the sibling example in 2D_phasechange_bubble.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
examples/2D_shocktube_phasechange/case.py (1)
7-7: Remove trailing whitespace.Line 7 contains trailing whitespace that violates linting rules.
🔎 Proposed fix
- formatter_class=argparse.ArgumentDefaultsHelpFormatter) + formatter_class=argparse.ArgumentDefaultsHelpFormatter)examples/3D_phasechange_bubble/case.py (1)
7-7: Remove trailing whitespace.Line 7 contains trailing whitespace that violates linting rules.
🔎 Proposed fix
- formatter_class=argparse.ArgumentDefaultsHelpFormatter) + formatter_class=argparse.ArgumentDefaultsHelpFormatter)
🧹 Nitpick comments (1)
examples/2D_phasechange_bubble/casefile.py (1)
307-309: Optional: Verify trailing newlines.Lines 307-309 appear to be blank lines at EOF. While past review comments indicate trailing newlines were addressed, ensure the file ends with a single newline per Python style guidelines.
🔎 Verification
Check the actual file ending:
#!/bin/bash # Check the last few bytes of the file to verify newline handling tail -c 20 examples/2D_phasechange_bubble/casefile.py | od -c
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
examples/1D_exp_tube_phasechange/case.py(2 hunks)examples/2D_phasechange_bubble/case.py(3 hunks)examples/2D_phasechange_bubble/casefile.py(3 hunks)examples/2D_shocktube_phasechange/case.py(2 hunks)examples/3D_phasechange_bubble/case.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- examples/2D_phasechange_bubble/case.py
- examples/1D_exp_tube_phasechange/case.py
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: CR
Repo: MFlowCode/MFC PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-24T21:50:16.713Z
Learning: Add and update focused tests for changes; ensure no regressions in golden test outputs without clear justification
🧬 Code graph analysis (2)
examples/3D_phasechange_bubble/case.py (1)
toolchain/mfc/args.py (2)
parse(10-193)add_common_arguments(50-77)
examples/2D_phasechange_bubble/casefile.py (1)
toolchain/mfc/args.py (2)
add_common_arguments(50-77)parse(10-193)
🪛 GitHub Actions: Lint Toolchain
examples/2D_shocktube_phasechange/case.py
[error] 7-7: Pylint: Trailing whitespace (C0303).
examples/3D_phasechange_bubble/case.py
[error] 7-7: Pylint: Trailing whitespace (C0303).
⏰ 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). (8)
- GitHub Check: Coverage Test on CodeCov
- GitHub Check: Code Cleanliness Check
- GitHub Check: Github (ubuntu, mpi, debug, true)
- GitHub Check: Github (ubuntu, mpi, no-debug, true)
- GitHub Check: Github (ubuntu, mpi, debug, false)
- GitHub Check: Github (ubuntu, no-mpi, single, no-debug, false)
- GitHub Check: Github (ubuntu, mpi, no-debug, false)
- GitHub Check: Build & Publish
🔇 Additional comments (3)
examples/2D_shocktube_phasechange/case.py (1)
2-12: LGTM: CLI-driven model selection implemented correctly.The argparse setup is well-structured with appropriate defaults (
default=3for 6-equation model), input validation (choices=[2, 3]), and clear help text. The integration at line 186 correctly replaces the hardcodedmodel_eqnsvalue with the CLI-provided argument.Also applies to: 186-186
examples/3D_phasechange_bubble/case.py (1)
2-12: LGTM: CLI implementation is correct and consistent.The argparse setup follows the same correct pattern as other phase-change examples, with appropriate defaults, validation, and integration at line 221.
Also applies to: 221-221
examples/2D_phasechange_bubble/casefile.py (1)
2-12: LGTM: CLI implementation is correct.The argparse setup is properly configured with
default=3, appropriate validation, and correct integration at line 222. The implementation is consistent with other phase-change examples in this PR.Also applies to: 222-222
2 participants
User description
Description
This PR extends the m_phase_change module to include the activation of pT- and pTg-relaxation schemes to the 5-equation model. It updates m_checker_common to include new constraints and includes examples for the new feature. New tests are also added
Type of change
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
pT:
pT-ET-5x6Eq-AlphaRho2.mp4
pT-ET-5x6Eq-p.mp4
pTg:
pTg-ET-5x6Eq-AlphaRho2.mp4
pTg-ET-5x6Eq-p.mp4
pT:
pT-ST-5x6Eq-AlphaRho2.mp4
pT-ST-5x6Eq-p.mp4
pTg:
pTg-ST-5x6Eq-AlphaRho2.mp4
pTg-ST-5x6Eq-p.mp4
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, Tests
Description
Extended phase change module to support 5-equation model with pT and pTg relaxation schemes, previously only available for 6-equation model
Fixed conditional internal energy assignment in
m_phase_change.fppto prevent calculation for 5-equation model which doesn't track individual fluid energiesUpdated phase change validation in
case_validator.pyto allow both 5-equation (model_eqns == 2) and 6-equation (model_eqns == 3) models with appropriate relaxation constraintsExtended test suite to generate tests for both 5 and 6-equation models by iterating over
model_eqnsvaluesAdded comprehensive example cases demonstrating phase change physics:
All examples configured with 3-fluid systems (liquid water, water vapor, air) and relaxation parameters for pressure and temperature equilibration
Validated through expansion tube and shock tube simulations showing consistent results between 5 and 6-equation models
Diagram Walkthrough
File Walkthrough
1 files
m_phase_change.fpp
Conditional internal energy assignment for phase change modelssrc/common/m_phase_change.fpp
model_eqns == 3before assigning internalenergy
q_cons_vf(i + intxb - 1)%sf== 2) which doesn't track individual fluid energies
19 files
cases.py
Extended phase change tests for 5 and 6-equation modelstoolchain/mfc/test/cases.py
model_eqnsvalues2 and 3
model_eqnsloop to generatetests for both 5-equation and 6-equation models
model_eqnsparameter indescription
golden-metadata.txt
Test metadata for phase change validationtests/8D0E7E1D/golden-metadata.txt
golden-metadata.txt
Test metadata for phase change validationtests/02BD9B5E/golden-metadata.txt
golden-metadata.txt
Test metadata for phase change validationtests/AAE5C27B/golden-metadata.txt
golden-metadata.txt
Test metadata for phase change validationtests/623B6C30/golden-metadata.txt
golden-metadata.txt
Test metadata file for 5-equation model relaxationtests/8D6E051E/golden-metadata.txt
build environment
and pre_process components
details
reproducibility
golden-metadata.txt
Test metadata file for 5-equation model relaxationtests/B2EFB9F7/golden-metadata.txt
build environment
and pre_process components
details
reproducibility
golden-metadata.txt
Test metadata file for 5-equation model relaxationtests/76C663B7/golden-metadata.txt
build environment
and pre_process components
details
reproducibility
golden-metadata.txt
Test metadata file for 5-equation model relaxationtests/D21F4F38/golden-metadata.txt
build environment
and pre_process components
details
reproducibility
golden-metadata.txt
Test metadata file for 5-equation model relaxationtests/E0256CBF/golden-metadata.txt
build environment
simulation components
details
reproducibility
golden-metadata.txt
Test metadata file for 5-equation model relaxationtests/2486442D/golden-metadata.txt
build environment
and syscheck components
details
reproducibility
golden-metadata.txt
Test metadata file for 5-equation model relaxationtests/60EE4401/golden-metadata.txt
build environment
and syscheck components
details
reproducibility
golden-metadata.txt
Test metadata file for 5-equation model relaxationtests/723DDBCF/golden-metadata.txt
build environment
and syscheck components
details
reproducibility
golden-metadata.txt
Test metadata file for 5-equation model relaxationtests/C46769FD/golden-metadata.txt
build environment
and simulation components
details
reproducibility
golden-metadata.txt
Test metadata file for 5-equation model relaxationtests/E6DBCBD9/golden-metadata.txt
build environment
and pre_process components
details
reproducibility
golden-metadata.txt
Test metadata file for 5-equation model relaxationtests/F4D00494/golden-metadata.txt
build environment
and pre_process components
details
reproducibility
golden-metadata.txt
Test metadata file for 5-equation model relaxationtests/344FAA70/golden-metadata.txt
build environment
and syscheck components
details
reproducibility
golden-metadata.txt
Test metadata file for 5-equation model relaxationtests/413DD555/golden-metadata.txt
build environment
and pre_process components
details
reproducibility
golden-metadata.txt
Test metadata documentation for 5-equation model relaxationtests/D8136392/golden-metadata.txt
system information
post_process, and pre_process components
flags (MPI enabled, OpenACC/OpenMP disabled)
reproducibility
6 files
casefile.py
2D phase change bubble example with 6-equation modelexamples/2D_phasechange_bubble_6Eqn/casefile.py
6-equation model
cylindrical coordinates
vapor bubble
equilibration
case.py
3D phase change bubble example with 6-equation modelexamples/3D_phasechange_bubble_6Eqn/case.py
6-equation model
Cartesian coordinates
center
equilibration
case.py
2D phase change bubble example with 6-equation modelexamples/2D_phasechange_bubble_6Eqn/case.py
6-equation model
Cartesian coordinates
center
equilibration
case.py
2D shock tube phase change example with 6-equation modelexamples/2D_shocktube_phasechange_6Eqn/case.py
6-equation model
patches
regions
equilibration
case.py
1D expansion tube phase change example with 5-equation modelexamples/1D_exp_tube_phasechange_5Eqn/case.py
5-equation model
patches
model_eqns == 2for 5-equation formulation with relaxationcase.py
1D expansion tube phase change example with 6-equation modelexamples/1D_exp_tube_phasechange_6Eqn/case.py
6-equation model
patches
model_eqns == 3for 6-equation formulation with relaxation1 files
case_validator.py
Phase change validation for 5 and 6-equation modelstoolchain/mfc/case_validator.py
model_eqns== 2) and 6-equation (model_eqns == 3) modelsrelax_model5 or 6 (pT and pTgrelaxation)
relax_model1, 4, 5, or 6covering all valid combinations
45 files
Summary by CodeRabbit
New Features
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.