fix swapped vapor gas properties #1093
Conversation
|
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 · |
|
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. WalkthroughRefactors bubble-parameter naming/layout and corrects vapor↔gas mappings in example cases, removes several fields from the Lagrange bubble parameter type and their MPI broadcasts, reorders test/tooling keys, adjusts docs formatting, and updates many golden-metadata snapshots to a different build/toolchain environment. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested reviewers
Poem
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 |
codeant-ai
bot
added
the
size:XL
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 🔍
|
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
![codeant-ai[bot]](https://avatars.githubusercontent.com/in/646884?s=60&v=4){kind=small}
|
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: 4
♻️ Duplicate comments (1)
tests/B9553426/golden-metadata.txt (1)
1-7: Same issue: Regenerate in CI environment.This golden metadata file exhibits the same concerns as tests/2122A4F6/golden-metadata.txt - local generation on M4Pro.local with dirty git state.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
examples/2D_lagrange_bubblescreen/case.py(3 hunks)examples/3D_lagrange_bubblescreen/case.py(1 hunks)examples/3D_lagrange_shbubcollapse/case.py(1 hunks)tests/016C1B8B/golden-metadata.txt(1 hunks)tests/0D1FA5C5/golden-metadata.txt(1 hunks)tests/12ECE133/golden-metadata.txt(1 hunks)tests/2122A4F6/golden-metadata.txt(1 hunks)tests/4A1BD9B8/golden-metadata.txt(1 hunks)tests/B9553426/golden-metadata.txt(1 hunks)tests/CE7B0BC7/golden-metadata.txt(1 hunks)tests/DFF6E349/golden-metadata.txt(1 hunks)toolchain/mfc/test/cases.py(1 hunks)
🧰 Additional context used
🧠 Learnings (3)
📚 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/0D1FA5C5/golden-metadata.txttests/2122A4F6/golden-metadata.txttests/B9553426/golden-metadata.txttests/DFF6E349/golden-metadata.txttests/CE7B0BC7/golden-metadata.txttests/4A1BD9B8/golden-metadata.txttests/016C1B8B/golden-metadata.txttests/12ECE133/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/0D1FA5C5/golden-metadata.txttests/2122A4F6/golden-metadata.txttests/B9553426/golden-metadata.txttests/DFF6E349/golden-metadata.txttests/CE7B0BC7/golden-metadata.txttests/4A1BD9B8/golden-metadata.txttests/016C1B8B/golden-metadata.txttests/12ECE133/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/0D1FA5C5/golden-metadata.txttests/2122A4F6/golden-metadata.txttests/B9553426/golden-metadata.txttests/DFF6E349/golden-metadata.txttests/CE7B0BC7/golden-metadata.txttests/4A1BD9B8/golden-metadata.txttests/016C1B8B/golden-metadata.txttests/12ECE133/golden-metadata.txt
⏰ 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). (14)
- GitHub Check: Coverage Test on CodeCov
- GitHub Check: Code Cleanliness Check
- GitHub Check: Github (macos, mpi, debug, false)
- GitHub Check: Github (ubuntu, mpi, no-debug, true)
- GitHub Check: Github (ubuntu, mpi, no-debug, false)
- GitHub Check: Github (ubuntu, mpi, debug, false)
- GitHub Check: Github (ubuntu, mpi, debug, true)
- GitHub Check: Github (ubuntu, no-mpi, single, no-debug, false)
- GitHub Check: Georgia Tech | Phoenix (gpu-acc)
- GitHub Check: Github (macos, mpi, no-debug, false)
- GitHub Check: Georgia Tech | Phoenix (gpu-omp)
- GitHub Check: Georgia Tech | Phoenix (cpu)
- GitHub Check: cubic · AI code reviewer
- GitHub Check: Build & Publish
🔇 Additional comments (15)
tests/0D1FA5C5/golden-metadata.txt (1)
1-153: Clarify the rationale for re-baselining the entire build environment.This file has been completely re-baselining from a Linux HPC environment to a macOS Apple Silicon (M4 Pro) environment. While the PR objectives mention updating golden files, the scope of this change—switching compilers, paths, CMake configuration, and CPU metadata across all sections—appears disproportionate to the stated goal of fixing swapped vapor/gas properties.
Key concerns:
- Scope mismatch: The PR focuses on parameter corrections; this file reflects a full environmental migration, not a minimal golden-file update.
- Reproducibility: If other CI/CD pipelines or developers run tests on Linux or different architectures, this baseline may cause failures or inconsistencies.
- Justification: The PR description does not explain why the entire environment baseline is being switched. Per the learnings, golden test output changes should include clear justification.
Please clarify:
- Is this re-baselining intentional and necessary for the bubble parameter fixes?
- Will this file be the new baseline for all future tests, or is it specific to your local development environment?
- Should golden metadata be environment-agnostic, or is it acceptable to lock golden tests to a specific machine/architecture?
If this re-baselining is required, consider documenting the reasoning in the PR description or commit message for future reviewers.
tests/DFF6E349/golden-metadata.txt (1)
1-154: This file is correctly included in version control.MFC's test infrastructure intentionally tracks
golden-metadata.txtfiles with environment-specific details. The.gitignoreexplicitly includes!/tests/*/golden-metadata.txt, requiring these files to be committed. Build environment and device metadata are captured by design to support benchmarking and cross-system regression testing in an HPC context. The presence of different hostnames and paths across test cases reflects the expected behavior of the test suite running on multiple systems.Likely an incorrect or invalid review comment.
tests/CE7B0BC7/golden-metadata.txt (3)
1-7: Clarify the scope and justification for golden-metadata.txt rebaseline.The PR objective is to "fix swapped vapor gas properties" but this golden file reflects a complete environmental/toolchain rebaseline from Linux HPC to macOS Apple (M4 Pro), including compiler changes, Fypp paths, and CPU metadata. This appears disconnected from the vapor/gas property fix.
Verify:
- Whether the gas/vapor property corrections actually require this golden-metadata rebaseline, or if this is incidental environmental drift.
- Whether the dirty state on line 7 should be cleaned before commit.
27-27: Explain the OpenMP toggle change from ON to OFF.All four component sections (simulation, syscheck, post_process, pre_process) show OpenMP switched from ON to OFF. This change is not mentioned in the PR summary. Clarify whether this is:
- Intentional as part of fixing the vapor/gas properties, or
- An incidental environmental quirk on the developer's macOS machine.
Also applies to: 61-61, 95-95, 129-129
145-154: Verify CPU metadata reflects the correct build environment.The CPU metadata changed from Linux/Intel-like to Apple M4 Pro (macOS). If this golden file is meant to capture canonical build information for CI/CD regression testing, ensure this reflects the primary target environment (likely Linux/HPC), not a local development machine.
examples/3D_lagrange_shbubcollapse/case.py (1)
167-176: LGTM! Vapor/gas property mappings corrected.The bubble parameter assignments now correctly map vapor properties (gam_v, M_v, k_v, cp_v, R_v) to their respective
bub_pp%*_vparameters and gas properties tobub_pp%*_gparameters, aligning with the fix described in the PR objectives.toolchain/mfc/test/cases.py (1)
865-868: LGTM! Test parameter keys reordered to match corrected vapor/gas mappings.The bub_pp parameter keys have been reordered to align vapor properties (M_v, k_v, R_v) before their gas counterparts (M_g, k_g, R_g), consistent with the vapor/gas swap fix across the PR.
examples/3D_lagrange_bubblescreen/case.py (1)
165-174: LGTM! Vapor/gas property mappings corrected.Bubble parameters correctly assign vapor and gas properties to their respective
bub_ppfields, consistent with the fix in other example files.examples/2D_lagrange_bubblescreen/case.py (3)
29-30: LGTM! Variable naming standardized.The renaming from
gamma_g/gamma_vtogam_g/gam_valigns with the naming convention used for other gas/vapor properties in the codebase.
131-162: LGTM! Bubble parameter block correctly structured.The new
bub_ppparameter block properly organizes all bubble-related properties with correct vapor/gas mappings:
- Vapor properties (gam_v, M_v, k_v, cp_v, R_v) →
bub_pp%*_v- Gas properties (gam_g, M_g, k_g, cp_g, R_g) →
bub_pp%*_gThe addition of
thermal=3andpolytropic="F"configures the non-polytropic thermal model.
169-169: LGTM! Fluid property updated to use new variable name.The update from
gamma_gtogam_gmaintains consistency with the variable renaming at the top of the file.tests/2122A4F6/golden-metadata.txt (1)
1-7: Regenerate golden metadata in CI environment.The golden metadata shows:
- Generated on local macOS machine (M4Pro.local)
- Git revision indicates "dirty" state with uncommitted changes
Golden test files should be generated in a clean CI environment to ensure consistency and avoid test failures when other contributors run tests on standard CI infrastructure.
Based on learnings, golden test outputs should match CI environment without local variations.
#!/bin/bash # Verify if other golden metadata files also show local/dirty generation echo "Checking for local machine names and dirty git states in golden metadata files:" fd -e txt golden-metadata.txt tests/ --exec grep -l "M4Pro.local\|dirty" {} \;⛔ Skipped due to 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 justificationtests/12ECE133/golden-metadata.txt (1)
1-10: Manual verification required for golden-metadata.txt changes.The review raises valid concerns about golden file management and environment-specific paths, aligned with best practices (auto-generated files should regenerate only when upstream behavior changes). However, verification is inconclusive without direct access to: (1) the actual committed paths in golden-metadata.txt, (2) the repository's path sanitization practices, and (3) the functional changes in the PR that triggered the metadata regeneration.
Request clarification on whether these metadata changes are necessary artifacts of the vapor/gas property fixes or unrelated infrastructure updates that should be decoupled.
tests/4A1BD9B8/golden-metadata.txt (2)
36-38: Verify environment-specific compiler paths are reproducible.Lines 36–38, 70–72, 104–106, and 138–140 hardcode absolute paths to compilers (
/usr/bin/cc,/opt/homebrew/bin/gfortran). These are environment-specific and may not be reproducible across different systems or CI environments. Verify that:
- This golden file is intended for this specific developer environment only, or
- Paths should be normalized/abstracted for cross-environment reproducibility.
Also applies to: 70-72, 104-106, 138-140
1-8: Clarify scope and legitimacy of golden file re-baselining.This golden file represents a significant re-baselining across multiple dimensions:
- Build environment migration (Linux → macOS Apple M4 Pro)
- CMake/toolchain versions updated
- Configuration section reordering
- User-specific paths introduced
The PR objectives focus on fixing swapped vapor/gas properties and correcting parameter inputs, but this file captures only build/environment metadata changes. Verify that:
- This re-baselining is intentional and necessary for the PR's correctness goals, and
- Other related golden files (and test outputs) in the PR follow the same environment migration pattern.
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 (5)
docs/documentation/case.md(1 hunks)src/common/m_derived_types.fpp(0 hunks)src/simulation/m_global_parameters.fpp(0 hunks)src/simulation/m_mpi_proxy.fpp(1 hunks)toolchain/mfc/run/case_dicts.py(1 hunks)
💤 Files with no reviewable changes (2)
- src/common/m_derived_types.fpp
- src/simulation/m_global_parameters.fpp
🧰 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/simulation/m_mpi_proxy.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.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/simulation/m_mpi_proxy.fpp
🧠 Learnings (7)
📚 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/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} : Declare loop-local variables with `private='[...]'` in GPU parallel loop macros
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: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 **/*.{fpp,f90} : Limit subroutines to ≤ 6 arguments; otherwise pass a derived-type 'params' struct
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
📚 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 **/*.{fpp,f90} : Limit routine arguments to ≤ 6; use derived-type params struct if more are needed
Applied to files:
src/simulation/m_mpi_proxy.fpp
🪛 markdownlint-cli2 (0.18.1)
docs/documentation/case.md
755-755: Hard tabs
Column: 67
(MD010, no-hard-tabs)
756-756: Hard tabs
Column: 65
(MD010, no-hard-tabs)
⏰ 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). (12)
- GitHub Check: Code Cleanliness Check
- GitHub Check: Coverage Test on CodeCov
- GitHub Check: Github (ubuntu, mpi, debug, false)
- GitHub Check: Github (macos, mpi, debug, false)
- GitHub Check: Github (ubuntu, no-mpi, single, no-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: Github (ubuntu, mpi, no-debug, true)
- GitHub Check: Github (macos, mpi, no-debug, false)
- GitHub Check: Georgia Tech | Phoenix (gpu-acc)
- GitHub Check: Build & Publish
🔇 Additional comments (3)
toolchain/mfc/run/case_dicts.py (1)
352-353: LGTM! Consistent parameter cleanup.The reduction of
lag_paramsreal-typed fields to onlyepsilonb,valmaxvoid, andcharwidthcorrectly aligns with the removal ofc0,rho0,T0,x0, andThostfrom thebubbles_lagrange_parametersderived type and their corresponding MPI broadcasts.src/simulation/m_mpi_proxy.fpp (1)
143-145: LGTM! MPI broadcast correctly narrowed.The MPI broadcast of
lag_paramsfields now correctly transmits onlyepsilonb,charwidth, andvalmaxvoid, consistent with the updatedbubbles_lagrange_parametersderived type and the parameter dictionary incase_dicts.py.docs/documentation/case.md (1)
758-777: Documentation updates look good.The percent-sign escaping (
%%) is correctly applied to allbub_ppparameter names for proper markdown table rendering. The parameter descriptions and footnote markers are clear and accurate.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #1093 +/- ##
==========================================
- Coverage 43.99% 43.98% -0.02%
==========================================
Files 71 71
Lines 20289 20284 -5
Branches 1982 1982
==========================================
- Hits 8927 8922 -5
Misses 10225 10225
Partials 1137 1137 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
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 · |
|
@wilfonba is this right? |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
This PR is being reviewed by Cursor Bugbot
Details
You are on the Bugbot Free tier. On this plan, Bugbot will review limited PRs each billing cycle.
To receive Bugbot reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial.
| "lag_params%write_bubbles": "F", | ||
| "lag_params%write_bubbles_stats": "F", | ||
| "lag_params%c0": c0, | ||
| "lag_params%rho0": rho0, |
There was a problem hiding this comment.
Missing charwidth parameter in 2D Lagrangian bubble example
The 2D Lagrangian bubble example no longer sets lag_params%charwidth, but this parameter is required for 2D non-cylindrical simulations. The simulation code uses charwidth for cell volume calculations and particle domain boundary checks in m_bubbles_EL.fpp. Without setting it, the default value of -1e6 is used, causing incorrect volume calculations (negative volumes) and domain checks. The variable z_virtual is still defined on line 52 but is now unused after this change removed "lag_params%charwidth": z_virtual / x0.
2 participants
User description
User description
Description
As mentioned in #1085 and #1091, the gas and vapor properties were swapped in the code before #1085. #1085 fixed this in the code, but in order to validate the other changes though the CI tests, the input parameters in examples and tests where
bubbles_lagrange = Twere intentionally swapped. This PR corrects the input parameters and the golden files are updated accordingly.With that, two minor fixes are included.
Fixes #(issue) [optional]
#1091
Type of change
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?
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
Bug fix
Description
Corrects swapped vapor and gas properties in Lagrangian bubble examples
Updates bubble parameter assignments to use correct variable mappings
Removes deprecated fluid_pp parameters for bubble properties
Regenerates golden test files with corrected parameters
Diagram Walkthrough
File Walkthrough
4 files
Fix swapped bubble gas and vapor propertiesCorrect vapor and gas property assignmentsCorrect vapor and gas property assignmentsUpdate test case bubble parameter values8 files
Regenerate golden metadata for test caseRegenerate golden metadata for test caseRegenerate golden metadata for test caseRegenerate golden metadata for test caseRegenerate golden metadata for test caseRegenerate golden metadata for test caseRegenerate golden metadata for test caseRegenerate golden metadata for test case8 files
CodeAnt-AI Description
fix swapped vapor and gas properties in Lagrangian bubble examples and tests
What Changed
Impact
✅ Correct bubble physical behavior in Lagrange examples✅ Matching test golden files for Lagrangian bubble cases✅ Clearer, consistent property assignment for vapor vs. gas💡 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
Bug Fixes
Refactor
Chores
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.
Note
Fix gas/vapor property assignments in Lagrangian bubble examples, migrate inputs to
bub_pp, remove unusedlag_paramsfields, and update docs/tests.gam_v/gam_g,M_v/M_g,k_v/k_g,cp_v/cp_g,R_v/R_g).thermal = 3,polytropic = F; set bubble inputs viabub_pp%*and remove obsolete per-fluid/lag fields; minor var rename (gamma_*->gam_*).lag_params%{c0,rho0,T0,Thost,x0}frombubbles_lagrange_parameters, defaults, and MPI broadcasts.bub_pp%%*indocs/documentation/case.md.Written by Cursor Bugbot for commit 0a24e3f. Configure here.