removing duplicated codes in mixture rules for EE bubbles #1071
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. WalkthroughThe changes consolidate bubble-specific variable conversion paths into a generic species-fraction computation routine. A new public subroutine Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
⏰ 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)
🔇 Additional comments (2)
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 |
PR Reviewer Guide 🔍Here are some key observations to aid the review process:
|
codeant-ai
bot
added
the
size:L
![qodo-merge-pro[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
There was a problem hiding this comment.
High-level Suggestion
The new s_compute_species_fraction subroutine has a confusing structure with a conditional override at the end. Refactor it to have clear, separate logic paths for each physical model to improve readability. [High-level, importance: 7]
Solution Walkthrough:
Before:
subroutine s_compute_species_fraction(...)
if (num_fluids == 1) then
alpha_rho_K(1) = ...
if (igr .or. bubbles_euler) then
alpha_K(1) = 1._wp
else
alpha_K(1) = ...
end if
else
! ... logic for num_fluids > 1
end if
if (mpp_lim) then
! ... apply limits and normalize alpha_K
end if
! Override for a specific case
if (num_fluids == 1 .and. bubbles_euler) then
alpha_K(1) = q_vf(advxb)%sf(k, l, r)
end if
end subroutine
After:
subroutine s_compute_species_fraction(...)
if (num_fluids == 1 .and. bubbles_euler) then
alpha_rho_K(1) = q_vf(contxb)%sf(k, l, r)
alpha_K(1) = q_vf(advxb)%sf(k, l, r)
else if (num_fluids == 1) then
alpha_rho_K(1) = ...
if (igr) then
alpha_K(1) = 1._wp
else
alpha_K(1) = ...
end if
else
! ... logic for num_fluids > 1
end if
if (mpp_lim) then
! ... apply limits and normalize alpha_K
end if
end subroutine
| alpha_K = alpha_K/max(sum(alpha_K), 1.e-16_wp) | ||
| end if | ||
|
|
||
| if (num_fluids == 1 .and. bubbles_euler) alpha_K(1) = q_vf(advxb)%sf(k, l, r) |
There was a problem hiding this comment.
Suggestion: In s_compute_species_fraction, prevent an unconditional overwrite of alpha_K(1) when bubbles_euler is true. Add a check for .not. igr to the condition to respect the logic for ideal gas relaxation. [possible issue, importance: 8]
| if (num_fluids == 1 .and. bubbles_euler) alpha_K(1) = q_vf(advxb)%sf(k, l, r) | |
| if (num_fluids == 1 .and. bubbles_euler .and. .not. igr) alpha_K(1) = q_vf(advxb)%sf(k, l, r) |
| n_tait = 1._wp/n_tait + 1._wp !make this the usual little 'gamma' | ||
| B_tait = B_tait*(n_tait - 1)/n_tait ! make this the usual pi_inf |
There was a problem hiding this comment.
Suggestion: Prevent a potential division-by-zero error in the calculation of n_tait and B_tait. Use max(n_tait, sgm_eps) in the denominator to ensure it is non-zero. [possible issue, importance: 8]
| n_tait = 1._wp/n_tait + 1._wp !make this the usual little 'gamma' | |
| B_tait = B_tait*(n_tait - 1)/n_tait ! make this the usual pi_inf | |
| n_tait = 1._wp/max(n_tait, sgm_eps) + 1._wp !make this the usual little 'gamma' | |
| B_tait = B_tait*(n_tait - 1)/max(n_tait, sgm_eps) ! make this the usual pi_inf |
![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
🧹 Nitpick comments (2)
src/common/m_variables_conversion.fpp (2)
1343-1351: Use consistent tolerance constant; clarifybubbles_euleralpha override.Two observations:
Inconsistent tolerance: Line 1348 uses hardcoded
1.e-16_wp, while the rest of the codebase usessgm_epsfor similar purposes (see lines 347, 379, 739).Confusing logic flow: For single-fluid
bubbles_euler,alpha_K(1)is set to1._wpat line 1323, then potentially processed bympp_lim, and finally overwritten at line 1351. Consider restructuring to make the intent clearer.Apply this diff for consistency:
if (mpp_lim) then do i = 1, num_fluids alpha_rho_K(i) = max(0._wp, alpha_rho_K(i)) alpha_K(i) = min(max(0._wp, alpha_K(i)), 1._wp) end do - alpha_K = alpha_K/max(sum(alpha_K), 1.e-16_wp) + alpha_K = alpha_K/max(sum(alpha_K), sgm_eps) end if - if (num_fluids == 1 .and. bubbles_euler) alpha_K(1) = q_vf(advxb)%sf(k, l, r) + ! For single-fluid bubbles_euler, alpha_K represents void fraction from advection variable + if (num_fluids == 1 .and. bubbles_euler) alpha_K(1) = q_vf(advxb)%sf(k, l, r)
1312-1318: Add Doxygen-style documentation for the new public subroutine.As a public subroutine called from multiple modules (per the AI summary:
m_hyperelastic.fpp,m_ibm.fpp,m_cbc.fpp, etc.), this should have documentation describing its purpose and parameters.Add documentation header:
!> Computes partial densities (alpha_rho_K) and volume fractions (alpha_K) !! for species at a given cell location, handling igr and bubbles_euler modes. !! @param q_vf Conservative or primitive variables !! @param k First-coordinate cell index !! @param l Second-coordinate cell index !! @param r Third-coordinate cell index !! @param alpha_rho_K Partial densities for each fluid (output) !! @param alpha_K Volume fractions for each fluid (output) subroutine s_compute_species_fraction(q_vf, k, l, r, alpha_rho_K, alpha_K)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
src/common/m_checker_common.fpp(1 hunks)src/common/m_variables_conversion.fpp(7 hunks)src/simulation/m_bubbles_EE.fpp(1 hunks)src/simulation/m_bubbles_EL.fpp(1 hunks)src/simulation/m_cbc.fpp(1 hunks)src/simulation/m_hyperelastic.fpp(1 hunks)src/simulation/m_ibm.fpp(0 hunks)src/simulation/m_sim_helpers.fpp(1 hunks)
💤 Files with no reviewable changes (1)
- src/simulation/m_ibm.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_hyperelastic.fppsrc/simulation/m_sim_helpers.fppsrc/simulation/m_cbc.fppsrc/simulation/m_bubbles_EL.fppsrc/simulation/m_bubbles_EE.fppsrc/common/m_variables_conversion.fppsrc/common/m_checker_common.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_hyperelastic.fppsrc/simulation/m_sim_helpers.fppsrc/simulation/m_cbc.fppsrc/simulation/m_bubbles_EL.fppsrc/simulation/m_bubbles_EE.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_hyperelastic.fppsrc/simulation/m_sim_helpers.fppsrc/simulation/m_cbc.fppsrc/simulation/m_bubbles_EL.fppsrc/simulation/m_bubbles_EE.fppsrc/common/m_variables_conversion.fppsrc/common/m_checker_common.fpp
🧠 Learnings (15)
📚 Learning: 2025-11-24T21:50:16.684Z
Learnt from: CR
Repo: MFlowCode/MFC PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-24T21:50:16.684Z
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_hyperelastic.fppsrc/simulation/m_sim_helpers.fppsrc/simulation/m_cbc.fppsrc/simulation/m_bubbles_EE.fppsrc/common/m_variables_conversion.fpp
📚 Learning: 2025-11-24T21:50:46.879Z
Learnt from: CR
Repo: MFlowCode/MFC PR: 0
File: .cursor/rules/mfc-agent-rules.mdc:0-0
Timestamp: 2025-11-24T21:50:46.879Z
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_hyperelastic.fppsrc/simulation/m_sim_helpers.fppsrc/simulation/m_cbc.fppsrc/simulation/m_bubbles_EE.fppsrc/common/m_variables_conversion.fpp
📚 Learning: 2025-11-24T21:50:46.879Z
Learnt from: CR
Repo: MFlowCode/MFC PR: 0
File: .cursor/rules/mfc-agent-rules.mdc:0-0
Timestamp: 2025-11-24T21:50:46.879Z
Learning: Applies to src/simulation/**/*.{fpp,f90} : Mark GPU-callable helpers with `$:GPU_ROUTINE(function_name='...', parallelism='[seq]')` immediately after declaration
Applied to files:
src/simulation/m_hyperelastic.fppsrc/simulation/m_sim_helpers.fppsrc/simulation/m_bubbles_EE.fppsrc/common/m_variables_conversion.fpp
📚 Learning: 2025-11-24T21:50:16.684Z
Learnt from: CR
Repo: MFlowCode/MFC PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-24T21:50:16.684Z
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_hyperelastic.fppsrc/simulation/m_sim_helpers.fppsrc/simulation/m_cbc.fppsrc/simulation/m_bubbles_EE.fppsrc/common/m_variables_conversion.fpp
📚 Learning: 2025-11-24T21:50:46.879Z
Learnt from: CR
Repo: MFlowCode/MFC PR: 0
File: .cursor/rules/mfc-agent-rules.mdc:0-0
Timestamp: 2025-11-24T21:50:46.879Z
Learning: Applies to src/simulation/**/*.{fpp,f90} : Declare loop-local variables with `private='[...]'` in GPU parallel loop macros
Applied to files:
src/simulation/m_hyperelastic.fppsrc/simulation/m_sim_helpers.fppsrc/simulation/m_cbc.fppsrc/simulation/m_bubbles_EE.fppsrc/common/m_variables_conversion.fpp
📚 Learning: 2025-11-24T21:50:46.879Z
Learnt from: CR
Repo: MFlowCode/MFC PR: 0
File: .cursor/rules/mfc-agent-rules.mdc:0-0
Timestamp: 2025-11-24T21:50:46.879Z
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_hyperelastic.fppsrc/simulation/m_sim_helpers.fppsrc/simulation/m_cbc.fppsrc/simulation/m_bubbles_EE.fpp
📚 Learning: 2025-11-24T21:50:16.684Z
Learnt from: CR
Repo: MFlowCode/MFC PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-24T21:50:16.684Z
Learning: Applies to src/simulation/**/*.{fpp,f90} : Avoid stop/error stop inside GPU device code
Applied to files:
src/simulation/m_hyperelastic.fppsrc/simulation/m_sim_helpers.fppsrc/common/m_variables_conversion.fpp
📚 Learning: 2025-11-24T21:50:16.684Z
Learnt from: CR
Repo: MFlowCode/MFC PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-24T21:50:16.684Z
Learning: Applies to src/simulation/**/*.{fpp,f90} : Allocate large GPU arrays with managed memory or move them into persistent !$acc enter data regions at start-up
Applied to files:
src/simulation/m_hyperelastic.fppsrc/common/m_variables_conversion.fpp
📚 Learning: 2025-11-24T21:50:46.879Z
Learnt from: CR
Repo: MFlowCode/MFC PR: 0
File: .cursor/rules/mfc-agent-rules.mdc:0-0
Timestamp: 2025-11-24T21:50:46.879Z
Learning: Applies to src/simulation/**/*.{fpp,f90} : Allocate large arrays with `managed` or move them into a persistent `$:GPU_ENTER_DATA(...)` region at start-up
Applied to files:
src/simulation/m_hyperelastic.fppsrc/common/m_variables_conversion.fpp
📚 Learning: 2025-11-24T21:50:46.879Z
Learnt from: CR
Repo: MFlowCode/MFC PR: 0
File: .cursor/rules/mfc-agent-rules.mdc:0-0
Timestamp: 2025-11-24T21:50:46.879Z
Learning: Applies to **/*.{fpp,f90} : Use `private` declaration followed by explicit `public` exports in modules
Applied to files:
src/common/m_variables_conversion.fpp
📚 Learning: 2025-11-24T21:50:16.684Z
Learnt from: CR
Repo: MFlowCode/MFC PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-24T21:50:16.684Z
Learning: Applies to **/*.{fpp,f90} : Name public subroutines with s_<verb>_<noun> pattern (e.g., s_compute_flux)
Applied to files:
src/common/m_variables_conversion.fpp
📚 Learning: 2025-11-24T21:50:46.879Z
Learnt from: CR
Repo: MFlowCode/MFC PR: 0
File: .cursor/rules/mfc-agent-rules.mdc:0-0
Timestamp: 2025-11-24T21:50:46.879Z
Learning: Applies to **/*.{fpp,f90} : Name public subroutines as `s_<verb>_<noun>` (e.g., `s_compute_flux`) and functions as `f_<verb>_<noun>`
Applied to files:
src/common/m_variables_conversion.fpp
📚 Learning: 2025-11-24T21:50:46.879Z
Learnt from: CR
Repo: MFlowCode/MFC PR: 0
File: .cursor/rules/mfc-agent-rules.mdc:0-0
Timestamp: 2025-11-24T21:50:46.879Z
Learning: Applies to src/**/*.fpp : Use fypp macro `@:ALLOCATE(var1, var2)` for device-aware allocation instead of standard Fortran `allocate`
Applied to files:
src/common/m_variables_conversion.fpp
📚 Learning: 2025-11-24T21:50:16.684Z
Learnt from: CR
Repo: MFlowCode/MFC PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-24T21:50:16.684Z
Learning: Applies to **/*.{fpp,f90} : Limit routine arguments to ≤ 6; use derived-type params struct if more are needed
Applied to files:
src/common/m_checker_common.fpp
📚 Learning: 2025-11-24T21:50:46.879Z
Learnt from: CR
Repo: MFlowCode/MFC PR: 0
File: .cursor/rules/mfc-agent-rules.mdc:0-0
Timestamp: 2025-11-24T21:50:46.879Z
Learning: Applies to **/*.{fpp,f90} : Limit subroutines to ≤ 6 arguments; otherwise pass a derived-type 'params' struct
Applied to files:
src/common/m_checker_common.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). (17)
- GitHub Check: cubic · AI code reviewer
- GitHub Check: Code Cleanliness Check
- GitHub Check: Github (ubuntu, no-mpi, single, no-debug, false)
- GitHub Check: Github (ubuntu, mpi, debug, false)
- GitHub Check: Github (ubuntu, mpi, no-debug, true)
- GitHub Check: Github (ubuntu, mpi, no-debug, false)
- GitHub Check: Self Hosted (gpu, acc, frontier)
- GitHub Check: Self Hosted (gpu, omp, gt)
- GitHub Check: Github (ubuntu, mpi, debug, true)
- GitHub Check: Github (macos, mpi, no-debug, false)
- GitHub Check: Self Hosted (gpu, omp, frontier)
- GitHub Check: Self Hosted (gpu, acc, gt)
- GitHub Check: Self Hosted (cpu, none, gt)
- GitHub Check: Self Hosted (cpu, none, frontier)
- GitHub Check: Github (macos, mpi, debug, false)
- GitHub Check: Coverage Test on CodeCov
- GitHub Check: Build & Publish
🔇 Additional comments (12)
src/common/m_checker_common.fpp (1)
314-333: LGTM - Loop bound expansion for EE bubbles validation.The change to set
bub_fac = 1wheneverbubbles_euleris true (regardless ofnum_fluids) correctly extends the validation loop to check the bubble gas fluid properties (fluid_pp(num_fluids + 1)). This aligns with the broader refactor to unify species fraction handling for EE bubbles.src/simulation/m_cbc.fpp (1)
816-816: LGTM - Unified conversion path for CBC.The removal of the bubbles_euler conditional branch in favor of unconditionally calling
s_convert_species_to_mixture_variables_acccorrectly adopts the unified species-to-mixture conversion approach. This aligns with the PR's goal of consolidating duplicated code paths.src/simulation/m_bubbles_EL.fpp (1)
645-648: LGTM - Centralized species fraction computation.The refactoring correctly delegates species fraction extraction to
s_compute_species_fraction, which populatesmyalpha_rhoandmyalphafor the given cell coordinates. The subsequent call tos_convert_species_to_mixture_variables_accconsumes these outputs appropriately. The variables are properly declared asprivatein the GPU parallel loop directive at line 620.src/simulation/m_bubbles_EE.fpp (1)
245-260: LGTM - Simplified single/multi-fluid handling.The refactored conditional correctly handles both cases:
- Single-fluid (
num_fluids == 1): Direct assignment from fluid properties.- Multi-fluid: Volume-fraction-weighted accumulation for
myRho,n_tait, andB_tait.The logic is clear and eliminates the previously duplicated code paths while maintaining correct behavior for both scenarios.
src/simulation/m_hyperelastic.fpp (1)
113-118: LGTM - Delegated species fraction computation to centralized routine.The replacement of the per-fluid loop with
s_compute_species_fraction(q_cons_vf, j, k, l, alpha_rho_k, alpha_k)correctly delegates species fraction extraction to the shared utility. The subsequent call tos_convert_species_to_mixture_variables_accproperly consumes the outputs, including the optionalG_localandGs_hyperarguments for hyperelasticity.src/simulation/m_sim_helpers.fpp (1)
112-119: LGTM - Unified species fraction extraction in enthalpy computation.The consolidation to a single
s_compute_species_fractioncall removes the previously branched logic for IGR vs non-IGR and bubbles-specific handling. The subsequent conversion path correctly uses the generals_convert_species_to_mixture_variables_accroutine for both elasticity and standard cases. This aligns with the PR's objective of centralizing species fraction computation.src/common/m_variables_conversion.fpp (6)
42-42: LGTM!The new subroutine
s_compute_species_fractionis correctly exported and follows thes_<verb>_<noun>naming convention per coding guidelines.
95-98: LGTM!Good simplification—the volume fraction model dispatch now delegates all special cases (including
bubbles_euler) to the unifieds_convert_species_to_mixture_variablessubroutine.
262-280: LGTM!The refactoring cleanly separates species fraction computation from mixture variable derivation. The special case for single-fluid
bubbles_eulerand the general multi-fluid accumulation logic are correctly preserved.
336-356: Potential inconsistency inmpp_limhandling for single-fluidbubbles_eulercase.In
s_compute_species_fraction(lines 1343-1349),mpp_limconstraints are applied unconditionally after computing species fractions. However, in this_accvariant, thempp_limlogic (lines 342-348) is only applied in the multi-fluid branch, skipping it for the single-fluidbubbles_eulercase.This could cause divergent behavior between the non-accelerated and accelerated code paths when
mpp_lim = .true.andnum_fluids == 1withbubbles_euler.Please verify if
mpp_limshould also be applied in the single-fluidbubbles_eulerbranch for consistency:if (num_fluids == 1 .and. bubbles_euler) then rho_K = alpha_rho_K(1) gamma_K = gammas(1) pi_inf_K = pi_infs(1) qv_K = qvs(1) + if (mpp_lim) then + alpha_rho_K(1) = max(0._wp, alpha_rho_K(1)) + alpha_K(1) = min(max(0._wp, alpha_K(1)), 1._wp) + end if else
617-617: LGTM!The call to
s_compute_species_fractionis correctly placed within the GPU parallel loop, and the subroutine has the appropriateGPU_ROUTINEdeclaration for sequential execution within the parallel region.
1218-1218: No substantive change.This appears to be a formatting adjustment only.
Nitpicks 🔍
|
|
CodeAnt AI finished reviewing your PR. |
hyeoksu-lee
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/common/m_variables_conversion.fpp (1)
1321-1352: Critical: Confusing conditional override creates ambiguous logic flow.The structure of this subroutine has a problematic conditional override pattern that was flagged in previous reviews:
- Lines 1323-1324: When
igr .or. bubbles_euleris true, setsalpha_K(1) = 1._wp- Line 1352: Unconditionally overrides
alpha_K(1)whennum_fluids == 1 .and. bubbles_eulerThis creates ambiguous logic where the same condition (
bubbles_euler) leads to different assignments. The override at line 1352 negates the assignment at line 1324 for thebubbles_eulercase.Issue 1: The override doesn't check
.not. igr, which means it could potentially conflict with theigrcase (though in practiceigrandbubbles_eulermay not coexist).Issue 2: The structure makes the code difficult to understand and maintain.
As suggested in previous reviews, refactor to have clear, separate logic paths:
- if (num_fluids == 1) then - alpha_rho_K(1) = q_vf(contxb)%sf(k, l, r) - if (igr .or. bubbles_euler) then - alpha_K(1) = 1._wp - else - alpha_K(1) = q_vf(advxb)%sf(k, l, r) - end if + if (num_fluids == 1 .and. bubbles_euler) then + alpha_rho_K(1) = q_vf(contxb)%sf(k, l, r) + alpha_K(1) = q_vf(advxb)%sf(k, l, r) + else if (num_fluids == 1) then + alpha_rho_K(1) = q_vf(contxb)%sf(k, l, r) + if (igr) then + alpha_K(1) = 1._wp + else + alpha_K(1) = q_vf(advxb)%sf(k, l, r) + end if else if (igr) then do i = 1, num_fluids - 1 alpha_rho_K(i) = q_vf(i)%sf(k, l, r) alpha_K(i) = q_vf(advxb + i - 1)%sf(k, l, r) end do alpha_rho_K(num_fluids) = q_vf(num_fluids)%sf(k, l, r) alpha_K(num_fluids) = 1._wp - sum(alpha_K(1:num_fluids - 1)) else do i = 1, num_fluids alpha_rho_K(i) = q_vf(i)%sf(k, l, r) alpha_K(i) = q_vf(advxb + i - 1)%sf(k, l, r) end do end if end if if (mpp_lim) then do i = 1, num_fluids alpha_rho_K(i) = max(0._wp, alpha_rho_K(i)) alpha_K(i) = min(max(0._wp, alpha_K(i)), 1._wp) end do alpha_K = alpha_K/max(sum(alpha_K), 1.e-16_wp) end if - - if (num_fluids == 1 .and. bubbles_euler) alpha_K(1) = q_vf(advxb)%sf(k, l, r)Alternatively, if there's a specific reason for the override (e.g., the
mpp_limnormalization), add a comment explaining why this is intentional.
🧹 Nitpick comments (1)
src/common/m_variables_conversion.fpp (1)
1344-1350: Consider using sgm_eps for consistency.Line 1349 uses
1.e-16_wpas the epsilon value for normalization, while line 293 in the same file usessgm_eps. Consider usingsgm_epsconsistently throughout the module for maintainability.Apply this diff to use the consistent epsilon value:
- alpha_K = alpha_K/max(sum(alpha_K), 1.e-16_wp) + alpha_K = alpha_K/max(sum(alpha_K), sgm_eps)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/common/m_variables_conversion.fpp(7 hunks)
🧰 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_variables_conversion.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_variables_conversion.fpp
🧠 Learnings (12)
📚 Learning: 2025-11-24T21:50:16.684Z
Learnt from: CR
Repo: MFlowCode/MFC PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-24T21:50:16.684Z
Learning: Applies to **/*.{fpp,f90} : Name public subroutines with s_<verb>_<noun> pattern (e.g., s_compute_flux)
Applied to files:
src/common/m_variables_conversion.fpp
📚 Learning: 2025-11-24T21:50:46.879Z
Learnt from: CR
Repo: MFlowCode/MFC PR: 0
File: .cursor/rules/mfc-agent-rules.mdc:0-0
Timestamp: 2025-11-24T21:50:46.879Z
Learning: Applies to **/*.{fpp,f90} : Each variable should have one clear purpose; do not use the same variable for multiple purposes
Applied to files:
src/common/m_variables_conversion.fpp
📚 Learning: 2025-11-24T21:50:46.879Z
Learnt from: CR
Repo: MFlowCode/MFC PR: 0
File: .cursor/rules/mfc-agent-rules.mdc:0-0
Timestamp: 2025-11-24T21:50:46.879Z
Learning: Applies to **/*.{fpp,f90} : Name public subroutines as `s_<verb>_<noun>` (e.g., `s_compute_flux`) and functions as `f_<verb>_<noun>`
Applied to files:
src/common/m_variables_conversion.fpp
📚 Learning: 2025-11-24T21:50:16.684Z
Learnt from: CR
Repo: MFlowCode/MFC PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-24T21:50:16.684Z
Learning: Applies to src/simulation/**/*.{fpp,f90} : Allocate large GPU arrays with managed memory or move them into persistent !$acc enter data regions at start-up
Applied to files:
src/common/m_variables_conversion.fpp
📚 Learning: 2025-11-24T21:50:46.879Z
Learnt from: CR
Repo: MFlowCode/MFC PR: 0
File: .cursor/rules/mfc-agent-rules.mdc:0-0
Timestamp: 2025-11-24T21:50:46.879Z
Learning: Applies to src/simulation/**/*.{fpp,f90} : Allocate large arrays with `managed` or move them into a persistent `$:GPU_ENTER_DATA(...)` region at start-up
Applied to files:
src/common/m_variables_conversion.fpp
📚 Learning: 2025-11-24T21:50:46.879Z
Learnt from: CR
Repo: MFlowCode/MFC PR: 0
File: .cursor/rules/mfc-agent-rules.mdc:0-0
Timestamp: 2025-11-24T21:50:46.879Z
Learning: Applies to src/simulation/**/*.{fpp,f90} : Mark GPU-callable helpers with `$:GPU_ROUTINE(function_name='...', parallelism='[seq]')` immediately after declaration
Applied to files:
src/common/m_variables_conversion.fpp
📚 Learning: 2025-11-24T21:50:46.879Z
Learnt from: CR
Repo: MFlowCode/MFC PR: 0
File: .cursor/rules/mfc-agent-rules.mdc:0-0
Timestamp: 2025-11-24T21:50:46.879Z
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/common/m_variables_conversion.fpp
📚 Learning: 2025-11-24T21:50:16.684Z
Learnt from: CR
Repo: MFlowCode/MFC PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-24T21:50:16.684Z
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_variables_conversion.fpp
📚 Learning: 2025-11-24T21:50:16.684Z
Learnt from: CR
Repo: MFlowCode/MFC PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-24T21:50:16.684Z
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/common/m_variables_conversion.fpp
📚 Learning: 2025-11-24T21:50:16.684Z
Learnt from: CR
Repo: MFlowCode/MFC PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-24T21:50:16.684Z
Learning: Applies to src/simulation/**/*.{fpp,f90} : Avoid stop/error stop inside GPU device code
Applied to files:
src/common/m_variables_conversion.fpp
📚 Learning: 2025-11-24T21:50:46.879Z
Learnt from: CR
Repo: MFlowCode/MFC PR: 0
File: .cursor/rules/mfc-agent-rules.mdc:0-0
Timestamp: 2025-11-24T21:50:46.879Z
Learning: Applies to src/simulation/**/*.{fpp,f90} : Declare loop-local variables with `private='[...]'` in GPU parallel loop macros
Applied to files:
src/common/m_variables_conversion.fpp
📚 Learning: 2025-11-24T21:50:46.879Z
Learnt from: CR
Repo: MFlowCode/MFC PR: 0
File: .cursor/rules/mfc-agent-rules.mdc:0-0
Timestamp: 2025-11-24T21:50:46.879Z
Learning: Applies to src/**/*.fpp : Use fypp macro `@:ALLOCATE(var1, var2)` for device-aware allocation instead of standard Fortran `allocate`
Applied to files:
src/common/m_variables_conversion.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). (11)
- GitHub Check: cubic · AI code reviewer
- GitHub Check: Github (macos, mpi, debug, false)
- GitHub Check: Github (macos, mpi, no-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, true)
- 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 (7)
src/common/m_variables_conversion.fpp (7)
42-42: LGTM: Public interface addition follows naming conventions.The new subroutine export follows the
s_<verb>_<noun>pattern as required by coding guidelines.
95-98: LGTM: Simplified conditional improves readability.The consolidation of volume fraction model handling (with and without bubbles) into a single path is cleaner and delegates the bubbles_euler-specific logic to the called subroutine.
284-295: LGTM: Defensive check for viscous calculations.The addition of the
if (viscous)conditional prevents unnecessary Reynolds number calculations when viscosity is disabled, which is a good practice.
617-617: LGTM: Consistent use of centralized fraction computation.The call to
s_compute_species_fractioncentralizes the logic and is consistent with the refactoring objectives.
1312-1318: LGTM: Proper GPU directive and intent declarations.The subroutine signature correctly uses
intent(out)for the computed fractions, and the GPU directive with sequential parallelism and inlining is appropriate for this helper function.
336-356: I apologize for the technical issue. The repository cloning failed, which prevents me from directly verifying the call sites and initialization patterns in the codebase.Given this constraint, let me attempt to understand the concern using what information is available in the review comment itself:
The review comment raises a valid concern about
intent(inout)parameters in Fortran. When a subroutine hasintent(inout)parameters, they must be initialized before the call. However, without access to:
- The subroutine signature showing the actual intent declarations
- All call sites of
s_convert_species_to_mixture_variables_acc- The initialization patterns in calling code
I cannot definitively verify whether this concern is substantiated or if it's a false alarm.
Verify the initialization of alpha_rho_K and alpha_K at all call sites of s_convert_species_to_mixture_variables_acc in src/common/m_variables_conversion.fpp and related files.
Without repository access, I cannot complete the verification. Please retry the script execution or provide direct access to:
- The subroutine signature (showing intent declarations for
alpha_rho_Kandalpha_K)- A list of all call sites to this subroutine
- The initialization code surrounding those calls
262-280: Repository access unavailable for verification.The refactored code calls
s_compute_species_fractionand then handles the single-fluidbubbles_eulercase specially. The verification request asks to ensure that thealpha_Kvalues computed bys_compute_species_fractionat line 1352 are compatible with the conditional logic at line 267.However, I cannot access the repository to examine the
s_compute_species_fractionimplementation and verify:
- How
alpha_Kis set for thenum_fluids == 1 .and. bubbles_eulercase- Whether the computed values are consistent with their usage in the subsequent accumulation logic
- Whether the single-fluid branch correctly bypasses the mixture calculation
Manual verification needed: Confirm that when
num_fluids == 1andbubbles_euler == true, thealpha_Karray values returned bys_compute_species_fractionare consistent with the direct property assignment in lines 268-271, and that the multi-fluid branch at lines 273-280 correctly accumulates properties weighted byalpha_K.
![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/m_variables_conversion.fpp">
<violation number="1" location="src/common/m_variables_conversion.fpp:1352">
P2: Single-fluid Eulerian bubble volume fraction bypasses the limiter, so `alpha_K(1)` can leave the physical [0,1] range and corrupt Reynolds/mixture calculations.</violation>
</file>
Reply to cubic to teach it or ask questions. Re-run a review with @cubic-dev-ai review this PR
| alpha_K = alpha_K/max(sum(alpha_K), 1.e-16_wp) | ||
| end if | ||
|
|
||
| if (num_fluids == 1 .and. bubbles_euler) alpha_K(1) = q_vf(advxb)%sf(k, l, r) |
There was a problem hiding this comment.
P2: Single-fluid Eulerian bubble volume fraction bypasses the limiter, so alpha_K(1) can leave the physical [0,1] range and corrupt Reynolds/mixture calculations.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/common/m_variables_conversion.fpp, line 1352:
<comment>Single-fluid Eulerian bubble volume fraction bypasses the limiter, so `alpha_K(1)` can leave the physical [0,1] range and corrupt Reynolds/mixture calculations.</comment>
<file context>
@@ -1575,6 +1309,50 @@ contains
+ alpha_K = alpha_K/max(sum(alpha_K), 1.e-16_wp)
+ end if
+
+ if (num_fluids == 1 .and. bubbles_euler) alpha_K(1) = q_vf(advxb)%sf(k, l, r)
+
+ end subroutine s_compute_species_fraction
</file context>
| if (num_fluids == 1 .and. bubbles_euler) alpha_K(1) = q_vf(advxb)%sf(k, l, r) | |
| if (num_fluids == 1 .and. bubbles_euler) alpha_K(1) = min(max(q_vf(advxb)%sf(k, l, r), 0._wp), 1._wp) |
✅ Addressed in 262e980
There was a problem hiding this comment.
3 issues found across 8 files
Prompt for AI agents (all 3 issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="src/common/m_variables_conversion.fpp">
<violation number="1" location="src/common/m_variables_conversion.fpp:276">
P1: `s_convert_species_to_mixture_variables` now includes the bubble species when computing `gamma`, `pi_inf`, and `qv`, so bubbles_euler cases with multiple fluids use void-fraction-weighted properties instead of the carrier-liquid values required by the pressure equation.</violation>
<violation number="2" location="src/common/m_variables_conversion.fpp:352">
P1: `s_convert_species_to_mixture_variables_acc` also mixes the bubble species into `gamma_K`, `pi_inf_K`, and `qv_K`, breaking the carrier-liquid property assumption for bubbles_euler flows on the accelerated path.</violation>
<violation number="3" location="src/common/m_variables_conversion.fpp:1351">
P2: This late assignment bypasses the limiter/normalization performed above, so any negative or >1 bubble volume fraction from `q_vf(advxb)` is written back into `alpha_K` and propagates to mixture property calculations; clamp the value or move the assignment before the limiter.</violation>
</file>
Reply to cubic to teach it or ask questions. Re-run a review with @cubic-dev-ai review this PR
| alpha_K(i) = min(max(0._wp, alpha_K(i)), 1._wp) | ||
| alpha_K_sum = alpha_K_sum + alpha_K(i) | ||
| rho_K = rho_K + alpha_rho_K(i) | ||
| gamma_K = gamma_K + alpha_K(i)*gammas(i) |
There was a problem hiding this comment.
P1: s_convert_species_to_mixture_variables_acc also mixes the bubble species into gamma_K, pi_inf_K, and qv_K, breaking the carrier-liquid property assumption for bubbles_euler flows on the accelerated path.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/common/m_variables_conversion.fpp, line 352:
<comment>`s_convert_species_to_mixture_variables_acc` also mixes the bubble species into `gamma_K`, `pi_inf_K`, and `qv_K`, breaking the carrier-liquid property assumption for bubbles_euler flows on the accelerated path.</comment>
<file context>
@@ -473,47 +321,40 @@ contains
- alpha_K(i) = min(max(0._wp, alpha_K(i)), 1._wp)
- alpha_K_sum = alpha_K_sum + alpha_K(i)
+ rho_K = rho_K + alpha_rho_K(i)
+ gamma_K = gamma_K + alpha_K(i)*gammas(i)
+ pi_inf_K = pi_inf_K + alpha_K(i)*pi_infs(i)
+ qv_K = qv_K + alpha_rho_K(i)*qvs(i)
</file context>
✅ Addressed in 262e980
| alpha_rho_K(i) = max(0._wp, alpha_rho_K(i)) | ||
| alpha_K(i) = min(max(0._wp, alpha_K(i)), 1._wp) | ||
| rho = rho + alpha_rho_K(i) | ||
| gamma = gamma + alpha_K(i)*gammas(i) |
There was a problem hiding this comment.
P1: s_convert_species_to_mixture_variables now includes the bubble species when computing gamma, pi_inf, and qv, so bubbles_euler cases with multiple fluids use void-fraction-weighted properties instead of the carrier-liquid values required by the pressure equation.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/common/m_variables_conversion.fpp, line 276:
<comment>`s_convert_species_to_mixture_variables` now includes the bubble species when computing `gamma`, `pi_inf`, and `qv`, so bubbles_euler cases with multiple fluids use void-fraction-weighted properties instead of the carrier-liquid values required by the pressure equation.</comment>
<file context>
@@ -379,72 +251,48 @@ contains
- alpha_rho_K(i) = max(0._wp, alpha_rho_K(i))
- alpha_K(i) = min(max(0._wp, alpha_K(i)), 1._wp)
+ rho = rho + alpha_rho_K(i)
+ gamma = gamma + alpha_K(i)*gammas(i)
+ pi_inf = pi_inf + alpha_K(i)*pi_infs(i)
+ qv = qv + alpha_rho_K(i)*qvs(i)
</file context>
✅ Addressed in 262e980
| alpha_K = alpha_K/max(sum(alpha_K), 1.e-16_wp) | ||
| end if | ||
|
|
||
| if (num_fluids == 1 .and. bubbles_euler) alpha_K(1) = q_vf(advxb)%sf(k, l, r) |
There was a problem hiding this comment.
P2: This late assignment bypasses the limiter/normalization performed above, so any negative or >1 bubble volume fraction from q_vf(advxb) is written back into alpha_K and propagates to mixture property calculations; clamp the value or move the assignment before the limiter.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/common/m_variables_conversion.fpp, line 1351:
<comment>This late assignment bypasses the limiter/normalization performed above, so any negative or >1 bubble volume fraction from `q_vf(advxb)` is written back into `alpha_K` and propagates to mixture property calculations; clamp the value or move the assignment before the limiter.</comment>
<file context>
@@ -1575,6 +1309,49 @@ contains
+ alpha_K = alpha_K/max(sum(alpha_K), 1.e-16_wp)
+ end if
+
+ if (num_fluids == 1 .and. bubbles_euler) alpha_K(1) = q_vf(advxb)%sf(k, l, r)
+
+ end subroutine s_compute_species_fraction
</file context>
| if (num_fluids == 1 .and. bubbles_euler) alpha_K(1) = q_vf(advxb)%sf(k, l, r) | |
| if (num_fluids == 1 .and. bubbles_euler) alpha_K(1) = min(max(q_vf(advxb)%sf(k, l, r), 0._wp), 1._wp) |
✅ Addressed in 262e980
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #1071 +/- ##
==========================================
+ Coverage 44.08% 44.16% +0.08%
==========================================
Files 71 71
Lines 20284 20197 -87
Branches 1974 1970 -4
==========================================
- Hits 8943 8921 -22
+ Misses 10209 10148 -61
+ Partials 1132 1128 -4 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Case checking moved to Python! (MFlowCode#1066)
|
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 · |
codeant-ai
bot
added
size:L
|
CodeAnt AI Incremental review completed. |
2 participants
User description
User description
Description
This PR removes duplicated codes by
s_convert_species_to_mixture_variables_bubblesintos_convert_species_to_mixture_variabless_convert_species_to_mixture_variables_bubbles_accintos_convert_species_to_mixture_variables_accs_compute_species_fractionType 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?
test suite passed on MacBook M4 Pro and Carpenter GNU
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
Removed duplicate subroutines for EE bubbles by merging
s_convert_species_to_mixture_variables_bubblesands_convert_species_to_mixture_variables_bubbles_accinto unified routinesIntroduced new
s_compute_species_fractionsubroutine to consolidate partial density and volume fraction computation logicSimplified mixture if-statements by eliminating special-case handling for bubbles_euler model
Removed redundant conditional compilation blocks with identical code in both branches
Updated checker to simplify bubble factor logic and remove redundant validation constraints
Diagram Walkthrough
File Walkthrough
m_checker_common.fpp
Simplify stiffened EOS checker logicsrc/common/m_checker_common.fpp
num_fluids == 1num_fluids + bub_facinstead of separatehandling
duplicated earlier checks
m_variables_conversion.fpp
Merge bubble-specific routines and extract fraction computationsrc/common/m_variables_conversion.fpp
s_convert_species_to_mixture_variables_bubblesands_convert_species_to_mixture_variables_bubbles_accs_compute_species_fractionsubroutines_convert_species_to_mixture_variables_bubblessubroutine (135 lines)
s_convert_species_to_mixture_variables_bubbles_accsubroutine (83 lines)
s_convert_species_to_mixture_variablesto use unified logicfor bubbles_euler case
s_convert_species_to_mixture_variables_accto handlebubbles_euler within main routine
s_compute_species_fractionsubroutine to extract commonfraction computation logic
#ifdef MFC_SIMULATION/#elseblocks with identicalallocations
compilation blocks
m_bubbles_EE.fpp
Simplify bubble mixture property calculationssrc/simulation/m_bubbles_EE.fpp
logic for single vs multiple fluids
mpp_limwith different fluid countsmyRhofromq_prim_vf(1)%sfmyRho,n_tait, andB_taitinto single if-elseblock
m_bubbles_EL.fpp
Use unified fraction computation routinesrc/simulation/m_bubbles_EL.fpp
s_compute_species_fractionm_cbc.fpp
Remove bubbles_euler special case handlingsrc/simulation/m_cbc.fpp
bubbles_eulerbefore mixturevariable conversion
s_convert_species_to_mixture_variables_accfor all cases
m_hyperelastic.fpp
Use unified fraction computation routinesrc/simulation/m_hyperelastic.fpp
s_compute_species_fractionm_ibm.fpp
Remove bubbles_euler special case handlingsrc/simulation/m_ibm.fpp
bubbles_eulerbefore mixturevariable conversion
s_convert_species_to_mixture_variables_accfor all cases
m_sim_helpers.fpp
Consolidate fraction extraction and mixture conversionsrc/simulation/m_sim_helpers.fpp
with call to
s_compute_species_fractionbubbles_eulerbefore mixturevariable conversion
s_convert_species_to_mixture_variables_accfor all cases
CodeAnt-AI Description
Correct mixture properties and species fractions for EE bubble simulations
What Changed
Impact
✅ More stable EE bubble simulations✅ Correct mixture properties for single-fluid bubble setups✅ Consistent mixture behavior at boundaries and immersed interfaces💡 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
Release Notes
✏️ Tip: You can customize this high-level summary in your review settings.