Refactor subgrid bubble models #1085
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. WalkthroughConsolidates sub-grid bubble parameters into a new derived type Changes
Sequence Diagram(s)sequenceDiagram
participant User as User / Case generator
participant StartUp as s_read_input / s_initialize_modules
participant Globals as m_global_parameters (bub_pp)
participant Helper as m_helper (s_initialize_bubbles_model)
participant MPI as MPI proxy
participant Simulation as m_bubbles / m_qbmm
User->>StartUp: provide case + bub_pp inputs
StartUp->>Globals: populate bub_pp fields (defaults & user)
StartUp->>Helper: call s_initialize_bubbles_model()
Helper->>Globals: initialize pb0, mass_g0, mass_v0, weights and derived scales
Helper->>Simulation: supply initialized bubble arrays/params
StartUp->>MPI: broadcast core fluid_pp fields
StartUp->>MPI: if bubbles_euler or bubbles_lagrange -> broadcast bub_pp fields
MPI->>Simulation: deliver bub_pp and fluid params to ranks
Simulation->>Simulation: use bub_pp in advance_step / EE/EL / QBMM flows
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
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.
13 issues found across 35 files
Prompt for AI agents (all 13 issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="src/common/m_helper.fpp">
<violation number="1" location="src/common/m_helper.fpp:194">
P2: Debug print statements should be removed from production code. These `print *` statements will output internal variables to stdout during execution, which is likely unintended behavior.</violation>
</file>
<file name="toolchain/mfc/run/case_dicts.py">
<violation number="1" location="toolchain/mfc/run/case_dicts.py:110">
P2: Consider extracting the duplicated `bub_pp%` variable list into a constant to avoid maintaining identical lists in three places. If the variable list needs to be updated, all three occurrences must be changed in sync, risking inconsistency.</violation>
</file>
<file name="examples/1D_poly_bubscreen/case.py">
<violation number="1" location="examples/1D_poly_bubscreen/case.py:37">
P2: Variable `nb = 3` is defined but unused - the config hardcodes `"nb": 1`. Either use the variable (`"nb": nb`) or remove the unused definition to avoid confusion.</violation>
</file>
<file name="examples/3D_lagrange_shbubcollapse/case.py">
<violation number="1" location="examples/3D_lagrange_shbubcollapse/case.py:167">
P1: Gas and vapor properties are systematically swapped: `bub_pp%gam_v` receives `gam_g` (gas value) and vice versa. Same pattern applies to M_v/M_g, k_v/k_g, cp_v/cp_g, and R_v/R_g. This appears to work around the underlying bug mentioned in the PR description, but makes the code confusing and will cause issues when that bug is fixed. Consider assigning matching properties (`bub_pp%gam_v: gam_v`, etc.) or add clear comments explaining the intentional swap.</violation>
</file>
<file name="src/simulation/m_bubbles_EE.fpp">
<violation number="1" location="src/simulation/m_bubbles_EE.fpp:270">
P1: The `myR < 0` check is placed before the `alf < small_alf` threshold check. When `alf` is very small (essentially no bubbles), `myR` could contain numerical noise or invalid values, but this shouldn't matter since we'd zero out all sources anyway. Moving this check inside the `else` branch (where `alf >= small_alf`) would prevent spurious aborts when bubble radius is irrelevant.</violation>
</file>
<file name="examples/2D_whale_bubble_annulus/case.py">
<violation number="1" location="examples/2D_whale_bubble_annulus/case.py:18">
P2: Variable `R0ref` is defined but never used. The configuration should use the variable instead of the literal `1.0` to maintain consistency and prevent future bugs if the value needs to be changed.</violation>
</file>
<file name="src/simulation/m_riemann_solvers.fpp">
<violation number="1" location="src/simulation/m_riemann_solvers.fpp:3032">
P2: Inconsistent index for right state: `qR_prim_rs${XYZ}$_vf(j, k, l, i)` should use `j + 1` to access the right state, matching the pattern used elsewhere (e.g., line 4 of this block uses `j + 1`). Using `j` will print incorrect debug information.</violation>
</file>
<file name="examples/0D_bubblecollapse_adap/case.py">
<violation number="1" location="examples/0D_bubblecollapse_adap/case.py:66">
P2: The `dt` variable is computed but never used - a hardcoded `0.0001` is used instead. Either use the computed value or remove the dead code.</violation>
</file>
<file name="examples/2D_bubbly_steady_shock/case.py">
<violation number="1" location="examples/2D_bubbly_steady_shock/case.py:155">
P0: Missing normalization for `pi_inf_l`. The old code normalized `B_tait` by `p0`, but the new formula uses dimensional `pi_inf_l` directly. This should be `gam_l * (pi_inf_l / p0) / (gam_l - 1.0)` to maintain consistency with the `delta` calculation on line 61 and preserve the expected non-dimensional value (~3.94 vs ~399114).</violation>
</file>
<file name="examples/3D_lagrange_bubblescreen/case.py">
<violation number="1" location="examples/3D_lagrange_bubblescreen/case.py:165">
P1: Parameter values are swapped: `bub_pp%gam_v` should receive `gam_v` (1.333) not `gam_g` (1.4). Currently assigning gas-specific heat ratio to the vapor parameter.</violation>
</file>
<file name="src/simulation/m_global_parameters.fpp">
<violation number="1" location="src/simulation/m_global_parameters.fpp:421">
P2: The module variable `Eu` is declared without initialization. If read before being explicitly set, it will contain an indeterminate value. Initialize it at declaration (e.g., `real(wp) :: Eu = dflt_real`) or ensure it's set in the default-values routine.</violation>
</file>
<file name="src/simulation/m_bubbles.fpp">
<violation number="1" location="src/simulation/m_bubbles.fpp:376">
P1: The mixture gas constant `fR_m` is computed as a sum of mass-weighted constants but not normalized by total mass. This will produce an incorrect value scale. Divide by `(fmass_g + fmass_v)` to compute the correct mass-weighted average: `fR_m = (fmass_g*R_g + fmass_v*R_v)/(fmass_g + fmass_v)`</violation>
</file>
<file name="src/simulation/m_rhs.fpp">
<violation number="1" location="src/simulation/m_rhs.fpp:658">
P3: Local variables `denom` and `nbubble` are declared but appear unused. If they are needed later, initialize them at declaration to avoid use-before-assignment bugs. Otherwise, remove the unused declaration.</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: 4
♻️ Duplicate comments (22)
examples/2D_whale_bubble_annulus/case.py (1)
18-19: VariableR0refdefined but not used in configuration.The variable
R0ref = 1.0is defined butbub_pp%R0refuses the literal1.0instead. For maintainability, use the variable:- "bub_pp%R0ref": 1.0, + "bub_pp%R0ref": R0ref,examples/3D_lagrange_bubblescreen/case.py (1)
165-174: Critical: Gas and vapor parameters are systematically swapped.All
_v(vapor) parameters receive_g(gas) values and vice versa. Given the variable definitions:
gam_g = 1.4(gas),gam_v = 1.333(vapor)MW_g = 28.0(gas/air),MW_v = 18.0(vapor/water)The assignments are inverted:
- "bub_pp%gam_v": gam_g, - "bub_pp%gam_g": gam_v, - "bub_pp%M_v": MW_g, - "bub_pp%M_g": MW_v, - "bub_pp%k_v": k_g * (T0 / (x0 * rho0 * c0 * c0 * c0)), - "bub_pp%k_g": k_v * (T0 / (x0 * rho0 * c0 * c0 * c0)), - "bub_pp%cp_v": cp_g * (T0 / (c0 * c0)), - "bub_pp%cp_g": cp_v * (T0 / (c0 * c0)), - "bub_pp%R_v": (R_uni / MW_g) * (T0 / (c0 * c0)), - "bub_pp%R_g": (R_uni / MW_v) * (T0 / (c0 * c0)), + "bub_pp%gam_v": gam_v, + "bub_pp%gam_g": gam_g, + "bub_pp%M_v": MW_v, + "bub_pp%M_g": MW_g, + "bub_pp%k_v": k_v * (T0 / (x0 * rho0 * c0 * c0 * c0)), + "bub_pp%k_g": k_g * (T0 / (x0 * rho0 * c0 * c0 * c0)), + "bub_pp%cp_v": cp_v * (T0 / (c0 * c0)), + "bub_pp%cp_g": cp_g * (T0 / (c0 * c0)), + "bub_pp%R_v": (R_uni / MW_v) * (T0 / (c0 * c0)), + "bub_pp%R_g": (R_uni / MW_g) * (T0 / (c0 * c0)),This will result in physically incorrect simulation behavior for heat/mass transfer models.
toolchain/mfc/test/cases.py (1)
849-869: Lagrangian test casebub_ppgas/vapor properties appear swapped.
This matches a previously flagged issue:_gfields get vapor values and_vfields get gas values.- 'bub_pp%gam_g': 1.33, 'bub_pp%gam_v': 1.4, - 'bub_pp%M_g': 18.02, 'bub_pp%M_v': 28.97, 'bub_pp%k_g': 5.618695895665441e-06, - 'bub_pp%k_v': 7.392868685947116e-06, 'bub_pp%R_g': 1347.810235139403, 'bub_pp%R_v': 838.3686723235085, - 'bub_pp%cp_v': 2921.2822272326243, 'bub_pp%cp_g': 6134.692677188511 + 'bub_pp%gam_g': 1.4, 'bub_pp%gam_v': 1.33, + 'bub_pp%M_g': 28.97, 'bub_pp%M_v': 18.02, 'bub_pp%k_g': 7.392868685947116e-06, + 'bub_pp%k_v': 5.618695895665441e-06, 'bub_pp%R_g': 838.3686723235085, 'bub_pp%R_v': 1347.810235139403, + 'bub_pp%cp_g': 2921.2822272326243, 'bub_pp%cp_v': 6134.692677188511examples/3D_lagrange_shbubcollapse/case.py (1)
158-176: Gas/vapor properties look swapped inbub_pp(likely incorrect).
bub_pp%*_vfields are receiving gas constants (*_g) and vice-versa (includingR_v/R_gusing the opposite molar weights). This matches a previously flagged issue in this file.- "bub_pp%gam_v": gam_g, - "bub_pp%gam_g": gam_v, - "bub_pp%M_v": MW_g, - "bub_pp%M_g": MW_v, - "bub_pp%k_v": k_g * (T0 / (x0 * rho0 * c0 * c0 * c0)), - "bub_pp%k_g": k_v * (T0 / (x0 * rho0 * c0 * c0 * c0)), - "bub_pp%cp_v": cp_g * (T0 / (c0 * c0)), - "bub_pp%cp_g": cp_v * (T0 / (c0 * c0)), - "bub_pp%R_v": (R_uni / MW_g) * (T0 / (c0 * c0)), - "bub_pp%R_g": (R_uni / MW_v) * (T0 / (c0 * c0)), + "bub_pp%gam_v": gam_v, + "bub_pp%gam_g": gam_g, + "bub_pp%M_v": MW_v, + "bub_pp%M_g": MW_g, + "bub_pp%k_v": k_v * (T0 / (x0 * rho0 * c0 * c0 * c0)), + "bub_pp%k_g": k_g * (T0 / (x0 * rho0 * c0 * c0 * c0)), + "bub_pp%cp_v": cp_v * (T0 / (c0 * c0)), + "bub_pp%cp_g": cp_g * (T0 / (c0 * c0)), + "bub_pp%R_v": (R_uni / MW_v) * (T0 / (c0 * c0)), + "bub_pp%R_g": (R_uni / MW_g) * (T0 / (c0 * c0)),examples/2D_bubbly_steady_shock/case.py (1)
154-155:fluid_pp(1)%pi_infis missing/ p0nondimensionalization.
This matches the previously flagged issue and will massively change the EOS stiffness.- "fluid_pp(1)%pi_inf": gam_l * (pi_inf_l) / (gam_l - 1.0), + "fluid_pp(1)%pi_inf": gam_l * (pi_inf_l / p0) / (gam_l - 1.0),src/simulation/m_qbmm.fpp (2)
767-774: Guard non-polytropic denominators against 0 to avoid NaNs/FP exceptions.
This matches the previously flagged issue; the risks are real in several divisions.- chi_vw = 1._wp/(1._wp + R_v/R_g*(pb(id1, id2, id3, j, q)/pv - 1._wp)) - x_vw = M_g*chi_vw/(M_v + (M_g - M_v)*chi_vw) - k_mw = x_vw*k_v(q)/(x_vw + (1._wp - x_vw)*phi_vg) + (1._wp - x_vw)*k_g(q)/(x_vw*phi_gv + 1._wp - x_vw) - rho_mw = pv/(chi_vw*R_v*Tw) - rhs_mv(id1, id2, id3, j, q) = -Re_trans_c(q)*((mv(id1, id2, id3, j, q)/(mv(id1, id2, id3, j, q) + mass_g0(q))) - chi_vw) - rhs_mv(id1, id2, id3, j, q) = rho_mw*rhs_mv(id1, id2, id3, j, q)/Pe_c/(1._wp - chi_vw)/abscX(j, q) - grad_T = -Re_trans_T(q)*((pb(id1, id2, id3, j, q)/pb0(q))*(abscX(j, q)/R0(q))**3*(mass_g0(q) + mass_v0(q))/(mass_g0(q) + mv(id1, id2, id3, j, q)) - 1._wp) + chi_vw = 1._wp/max( & + 1._wp + R_v/R_g*(pb(id1, id2, id3, j, q)/pv - 1._wp), sgm_eps) + x_vw = M_g*chi_vw/max(M_v + (M_g - M_v)*chi_vw, sgm_eps) + k_mw = x_vw*k_v(q)/max(x_vw + (1._wp - x_vw)*phi_vg, sgm_eps) + & + (1._wp - x_vw)*k_g(q)/max(x_vw*phi_gv + 1._wp - x_vw, sgm_eps) + rho_mw = pv/max(chi_vw*R_v*Tw, sgm_eps) + rhs_mv(id1, id2, id3, j, q) = -Re_trans_c(q)*((mv(id1, id2, id3, j, q)/ & + max(mv(id1, id2, id3, j, q) + mass_g0(q), sgm_eps)) - chi_vw) + rhs_mv(id1, id2, id3, j, q) = rho_mw*rhs_mv(id1, id2, id3, j, q)/Pe_c/ & + max(1._wp - chi_vw, sgm_eps)/max(abscX(j, q), sgm_eps) + grad_T = -Re_trans_T(q)*((pb(id1, id2, id3, j, q)/pb0(q))* & + (abscX(j, q)/R0(q))**3*(mass_g0(q) + mass_v0(q))/ & + max(mass_g0(q) + mv(id1, id2, id3, j, q), sgm_eps) - 1._wp) ht(j, q) = pb0(q)*k_mw*grad_T/Pe_T(q)/abscX(j, q)
955-956: Use scalarmaxinstead ofmaxvalarray constructor forc2clamp.
This matches the previously flagged cleanup.- c2 = maxval((/c2, sgm_eps/)) + c2 = max(c2, sgm_eps)examples/0D_bubblecollapse_adap/case.py (1)
49-53: dt is computed but not used (hardcoded dt in JSON).
This still computesdt = Tfinal / (Nt - 1)but emits"dt": 0.0001. Either outputdtor drop the computeddtto avoid dead code / confusion.- "dt": 0.0001, + "dt": dt,Also applies to: 66-66
src/simulation/m_time_steppers.fpp (1)
515-516: Remove unused localsdenomandnbubble(or use them).
They’re declared but not referenced ins_tvd_rkin the shown code.- real(wp) :: denom, nbubblesrc/common/m_variables_conversion.fpp (1)
554-557: Guard against divide-by-zero in Preston pb init (mass_g0 + mass_v0).
Even if “shouldn’t happen”, this is a cheap defensive clamp.- pb(j, k, l, 1, i) = (pb0(i))*(R0(i)**(3._wp))*(mass_g0(i) + mv(j, k, l, 1, i))/(mu - sig)**(3._wp)/(mass_g0(i) + mass_v0(i)) - pb(j, k, l, 2, i) = (pb0(i))*(R0(i)**(3._wp))*(mass_g0(i) + mv(j, k, l, 2, i))/(mu - sig)**(3._wp)/(mass_g0(i) + mass_v0(i)) - pb(j, k, l, 3, i) = (pb0(i))*(R0(i)**(3._wp))*(mass_g0(i) + mv(j, k, l, 3, i))/(mu + sig)**(3._wp)/(mass_g0(i) + mass_v0(i)) - pb(j, k, l, 4, i) = (pb0(i))*(R0(i)**(3._wp))*(mass_g0(i) + mv(j, k, l, 4, i))/(mu + sig)**(3._wp)/(mass_g0(i) + mass_v0(i)) + pb(j, k, l, 1, i) = (pb0(i))*(R0(i)**(3._wp))*(mass_g0(i) + mv(j, k, l, 1, i))/(mu - sig)**(3._wp)/max(mass_g0(i) + mass_v0(i), sgm_eps) + pb(j, k, l, 2, i) = (pb0(i))*(R0(i)**(3._wp))*(mass_g0(i) + mv(j, k, l, 2, i))/(mu - sig)**(3._wp)/max(mass_g0(i) + mass_v0(i), sgm_eps) + pb(j, k, l, 3, i) = (pb0(i))*(R0(i)**(3._wp))*(mass_g0(i) + mv(j, k, l, 3, i))/(mu + sig)**(3._wp)/max(mass_g0(i) + mass_v0(i), sgm_eps) + pb(j, k, l, 4, i) = (pb0(i))*(R0(i)**(3._wp))*(mass_g0(i) + mv(j, k, l, 4, i))/(mu + sig)**(3._wp)/max(mass_g0(i) + mass_v0(i), sgm_eps)src/simulation/m_bubbles_EL.fpp (1)
345-347: LGTM: Mass diffusivity usesvd.The rename from
D_vtovdis consistent with the newbub_ppstructure.Note: Past review comments flagged potential division-by-zero risks for
kparticle(Line 341) andvd(Line 345). These concerns remain valid but are pre-existing logic issues, not introduced by this refactor.src/simulation/m_bubbles_EE.fpp (1)
270-272:s_mpi_abortinside GPU parallel loop is invalid.Calling
s_mpi_abortfrom within a GPU parallel region is problematic - host-side MPI calls cannot execute on GPU device code. Additionally, whenalf < small_alf, the value ofmyRmay contain numerical noise that would trigger a spurious abort.Consider using a reduction-based error flag (similar to
adap_dt_stop_max) and checking after the loop completes. As per coding guidelines, avoidstop/error stopinside GPU device code.src/pre_process/m_assign_variables.fpp (1)
421-450: QBMM moment-initialization code duplication persists.The nearly identical QBMM moment-initialization blocks at lines 421-450 (smoothing patch) and lines 628-658 (main patch) duplicate the same logic. This was flagged in a previous review. Consider extracting this into a helper subroutine to reduce maintenance burden and ensure consistent updates.
Also applies to: 628-658
toolchain/mfc/run/case_dicts.py (1)
110-114: Extract duplicatedbub_pp%variable list into a constant.The same list of 20
bub_pp%variables is defined identically in PRE_PROCESS (lines 110-113), SIMULATION (lines 362-365), and POST_PROCESS (lines 514-517). If this list needs updating, all three locations must be modified in sync, risking inconsistency.Consider extracting to a constant:
# Define once at module level BUB_PP_VARS = ["R0ref", "p0ref", "rho0ref", "T0ref", "ss", "pv", "vd", "mu_l", "mu_v", "mu_g", "gam_v", "gam_g", "M_v", "M_g", "k_v", "k_g", "cp_v", "cp_g", "R_v", "R_g"] # Then use in each section: for var in BUB_PP_VARS: PRE_PROCESS[f"bub_pp%{var}"] = ParamType.REALAlso applies to: 362-366, 514-518
src/common/m_helper.fpp (1)
241-248: Logic issue:Im_trans_Tis zeroed after computation.The loop at lines 241-246 computes
Im_trans_T(ir)vias_transcoeff, but line 247 immediately overwrites the entire array with zeros, discarding the computed values. This appears to be a bug.If zeroing is intentional (e.g., the imaginary part should be ignored), consider moving the initialization before the loop or adding a comment explaining the intent:
+ ! Note: Imaginary transport coefficients are intentionally zeroed + Im_trans_T = 0._wp do ir = 1, nb call s_transcoeff(omegaN(ir)*R0(ir), Pe_T(ir)*R0(ir), & Re_trans_T(ir), Im_trans_T(ir)) call s_transcoeff(omegaN(ir)*R0(ir), Pe_c*R0(ir), & Re_trans_c(ir), Im_trans_c(ir)) end do - Im_trans_T = 0._wpOr if the imaginary part should be preserved, simply remove line 247.
examples/1D_poly_bubscreen/case.py (1)
37-37: Unused variablenb = 3conflicts with hardcoded"nb": 1in config.This was flagged in a previous review. Either use the variable (
"nb": nb) or remove the unused definition to avoid confusion.src/post_process/m_global_parameters.fpp (2)
309-309: Verify initialization ofEu,Ca,Web,Re_inv.These scalar bubble parameters are declared but the visible code doesn't show their initialization in
s_assign_default_values_to_user_inputs. Ensure they are initialized todflt_realin the defaults subroutine to prevent undefined behavior if read before assignment.
323-323: Initialize module-levelGto prevent undefined reads.The scalar
Gis declared but not initialized ins_assign_default_values_to_user_inputs. AddG = dflt_realin the defaults subroutine for consistency with other module variables.src/simulation/m_global_parameters.fpp (2)
421-421: VerifyEuis initialized before use.The module variable
Euis declared but not visibly initialized ins_assign_default_values_to_user_inputs. Ensure it's set to a default value (e.g.,Eu = dflt_real) to prevent undefined reads.
482-482: Verifygam_minitialization.
gamis properly assigned frombub_pp%gam_gins_initialize_global_parameters_module, butgam_mis not visibly initialized. Sincegam_mis used in bubble pressure calculations (f_bpres_dot), ensure it's initialized either in the defaults subroutine or via the bubble model initialization pathway.src/simulation/m_bubbles.fpp (2)
376-377: Mixture gas constantfR_mcalculation may be incorrect.The mixture gas constant should be a mass-weighted average:
(m_g*R_g + m_v*R_v) / (m_g + m_v). The current code computes a sum without normalization, which will produce a value scaled by total mass rather than the correct averaged gas constant.- fR_m = (fmass_g*R_g + fmass_v*R_v) + fR_m = (fmass_g*R_g + fmass_v*R_v)/(fmass_g + fmass_v)
360-361: Optional arguments used withoutpresent()guards.
fmass_g,fbeta_c,fR_m, andfgamma_mare optional but used unguarded inside thebubbles_lagrangebranch. If called withbubbles_lagrange=.true.but without these arguments, undefined reads/writes occur. Either make them required or addpresent()guards.
🧹 Nitpick comments (8)
examples/2D_bubbly_steady_shock/case.py (1)
5-29: Unit annotations forR_uni,M_v/M_gare inconsistent with the numeric conventions used.
You’re usingR_uni = 8314.0withM_* = 18.02/28.97, which is consistent withR_uniin J/(kmol·K) andM_*in kg/kmol (not J/mol/K and g/mol).src/simulation/m_qbmm.fpp (1)
448-453: Good: flooringvarwithsgm_eps; consider also guardingAXbefore it’s used as a divisor.
AX = R - sqrt(var)can approach 0 (or go negative) and later appears in denominators; clamping can prevent inf/NaN blowups.- if (q <= 2) then - AX = R - sqrt(var) - else - AX = R + sqrt(var) - end if + if (q <= 2) then + AX = R - sqrt(var) + else + AX = R + sqrt(var) + end if + AX = max(AX, sgm_eps)examples/1D_qbmm/case.py (2)
14-23: Make the thermophysical units explicit (prevents silent scaling mistakes in bub_pp normalization).
Right nowk_v/k_gare commented as[]but are used as thermal conductivities in the nondimensionalization. Same forvd(looks like a diffusivity). Suggest tightening comments to match the math.-vd = 0.242e-4 # [] +vd = 0.242e-4 # [m^2/s] (if this is diffusivity; scaling by x0*u0 implies so) ... -k_v = 0.019426 # [] +k_v = 0.019426 # [W/m/K] ... -k_g = 0.02556 # [] +k_g = 0.02556 # [W/m/K]Also applies to: 171-176
141-142: bub_pp mapping looks consistent; consider consolidating repeated scaling factors for readability.
Thefluid_pp(1)%gamma/pi_infmapping matches the gamma/pi_inf model convention, and the bub_pp nondimensionalization is internally consistent. A small readability win would be factoring shared scales (e.g.,inv_rho0x0u0 = 1/(rho0*x0*u0),T0_over_u02 = T0/(u0*u0)).Also applies to: 157-176
src/pre_process/m_assign_variables.fpp (1)
704-712: Remove commented-out debug code.These commented-out print statements appear to be debugging artifacts and should be removed before merging to keep the codebase clean.
- ! if (j == 1) then - ! print *, (q_prim_vf(bub_idx%rs(i))%sf(j, k, l), i = 1, nb) - ! print *, (q_prim_vf(bub_idx%fullmom(i, 1, 0))%sf(j, k, l), i = 1, nb) - ! print *, (R0(i), i = 1, nb) - ! print *, patch_icpp(patch_id)%r0 - ! print *, (bub_idx%rs(i), i = 1, nb) - ! print *, (bub_idx%fullmom(i, 1, 0), i = 1, nb) - ! end if -src/post_process/m_global_parameters.fpp (1)
448-449: Minor style inconsistency: trailing semicolons without subsequent statements.Lines 448-449 have
bub_pp%k_v = dflt_real;andbub_pp%k_g = dflt_real;with trailing semicolons but no corresponding module-level scalar assignments (unlike other fields). This is correct since the module-levelk_vandk_gare allocatable arrays, not scalars, but consider removing the trailing semicolons for consistency.- bub_pp%k_v = dflt_real; - bub_pp%k_g = dflt_real; + bub_pp%k_v = dflt_real + bub_pp%k_g = dflt_realsrc/pre_process/m_global_parameters.fpp (1)
608-609: Minor style inconsistency: trailing semicolons without subsequent statements.Same as in post_process file - consider removing trailing semicolons for consistency.
- bub_pp%k_v = dflt_real; - bub_pp%k_g = dflt_real; + bub_pp%k_v = dflt_real + bub_pp%k_g = dflt_realsrc/simulation/m_global_parameters.fpp (1)
697-698: Minor style inconsistency: trailing semicolons without subsequent statements.Same pattern as other files - consider removing trailing semicolons for consistency.
- bub_pp%k_v = dflt_real; - bub_pp%k_g = dflt_real; + bub_pp%k_v = dflt_real + bub_pp%k_g = dflt_real
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (33)
examples/0D_bubblecollapse_adap/case.py(3 hunks)examples/1D_bubblescreen/case.py(4 hunks)examples/1D_exp_bubscreen/case.py(3 hunks)examples/1D_poly_bubscreen/case.py(2 hunks)examples/1D_qbmm/case.py(2 hunks)examples/2D_bubbly_steady_shock/case.py(4 hunks)examples/2D_whale_bubble_annulus/case.py(3 hunks)examples/3D_lagrange_bubblescreen/case.py(3 hunks)examples/3D_lagrange_shbubcollapse/case.py(3 hunks)src/common/m_derived_types.fpp(1 hunks)src/common/m_helper.fpp(2 hunks)src/common/m_variables_conversion.fpp(1 hunks)src/post_process/m_global_parameters.fpp(3 hunks)src/post_process/m_mpi_proxy.fpp(1 hunks)src/post_process/m_start_up.fpp(2 hunks)src/pre_process/m_assign_variables.fpp(4 hunks)src/pre_process/m_global_parameters.fpp(5 hunks)src/pre_process/m_mpi_proxy.fpp(2 hunks)src/pre_process/m_start_up.fpp(2 hunks)src/simulation/m_bubbles.fpp(21 hunks)src/simulation/m_bubbles_EE.fpp(1 hunks)src/simulation/m_bubbles_EL.fpp(2 hunks)src/simulation/m_global_parameters.fpp(6 hunks)src/simulation/m_mpi_proxy.fpp(1 hunks)src/simulation/m_qbmm.fpp(5 hunks)src/simulation/m_rhs.fpp(0 hunks)src/simulation/m_start_up.fpp(3 hunks)src/simulation/m_time_steppers.fpp(1 hunks)tests/5CAA4E68/golden-metadata.txt(8 hunks)toolchain/mfc/case_validator.py(1 hunks)toolchain/mfc/run/case_dicts.py(6 hunks)toolchain/mfc/test/case.py(1 hunks)toolchain/mfc/test/cases.py(4 hunks)
💤 Files with no reviewable changes (1)
- src/simulation/m_rhs.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_time_steppers.fppsrc/pre_process/m_start_up.fppsrc/simulation/m_mpi_proxy.fppsrc/common/m_derived_types.fppsrc/pre_process/m_mpi_proxy.fppsrc/common/m_variables_conversion.fppsrc/post_process/m_start_up.fppsrc/common/m_helper.fppsrc/simulation/m_qbmm.fppsrc/simulation/m_bubbles_EL.fppsrc/pre_process/m_assign_variables.fppsrc/simulation/m_global_parameters.fppsrc/pre_process/m_global_parameters.fppsrc/simulation/m_bubbles.fppsrc/simulation/m_bubbles_EE.fppsrc/post_process/m_mpi_proxy.fppsrc/simulation/m_start_up.fppsrc/post_process/m_global_parameters.fpp
src/simulation/**/*.{fpp,f90}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
src/simulation/**/*.{fpp,f90}: Wrap tight GPU loops with !$acc parallel loop gang vector default(present) reduction(...); add collapse(n) when safe; declare loop-local variables with private(...)
Allocate large GPU arrays with managed memory or move them into persistent !$acc enter data regions at start-up
Avoid stop/error stop inside GPU device code
Ensure GPU code compiles with Cray ftn, NVIDIA nvfortran, GNU gfortran, and Intel ifx/ifort compilers
src/simulation/**/*.{fpp,f90}: Mark GPU-callable helpers with$:GPU_ROUTINE(function_name='...', parallelism='[seq]')immediately after declaration
Do not use OpenACC or OpenMP directives directly; use Fypp macros fromsrc/common/include/parallel_macros.fppinstead
Wrap tight loops with$:GPU_PARALLEL_FOR(private='[...]', copy='[...]')macro; addcollapse=nfor safe nested loop merging
Declare loop-local variables withprivate='[...]'in GPU parallel loop macros
Allocate large arrays withmanagedor move them into a persistent$:GPU_ENTER_DATA(...)region at start-up
Do not placestoporerror stopinside device code
Files:
src/simulation/m_time_steppers.fppsrc/simulation/m_mpi_proxy.fppsrc/simulation/m_qbmm.fppsrc/simulation/m_bubbles_EL.fppsrc/simulation/m_global_parameters.fppsrc/simulation/m_bubbles.fppsrc/simulation/m_bubbles_EE.fppsrc/simulation/m_start_up.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_time_steppers.fppsrc/pre_process/m_start_up.fppsrc/simulation/m_mpi_proxy.fppsrc/common/m_derived_types.fppsrc/pre_process/m_mpi_proxy.fppsrc/common/m_variables_conversion.fppsrc/post_process/m_start_up.fppsrc/common/m_helper.fppsrc/simulation/m_qbmm.fppsrc/simulation/m_bubbles_EL.fppsrc/pre_process/m_assign_variables.fppsrc/simulation/m_global_parameters.fppsrc/pre_process/m_global_parameters.fppsrc/simulation/m_bubbles.fppsrc/simulation/m_bubbles_EE.fppsrc/post_process/m_mpi_proxy.fppsrc/simulation/m_start_up.fppsrc/post_process/m_global_parameters.fpp
🧠 Learnings (29)
📚 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_time_steppers.fppsrc/simulation/m_mpi_proxy.fppsrc/pre_process/m_mpi_proxy.fppsrc/common/m_variables_conversion.fppsrc/common/m_helper.fppsrc/simulation/m_qbmm.fppsrc/pre_process/m_assign_variables.fppsrc/simulation/m_global_parameters.fppsrc/simulation/m_bubbles.fppsrc/simulation/m_bubbles_EE.fppsrc/post_process/m_mpi_proxy.fppsrc/simulation/m_start_up.fpp
📚 Learning: 2025-11-24T21:50:16.713Z
Learnt from: CR
Repo: MFlowCode/MFC PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-24T21:50:16.713Z
Learning: Applies to src/simulation/**/*.{fpp,f90} : Wrap tight GPU loops with !$acc parallel loop gang vector default(present) reduction(...); add collapse(n) when safe; declare loop-local variables with private(...)
Applied to files:
src/simulation/m_time_steppers.fppsrc/simulation/m_mpi_proxy.fppsrc/pre_process/m_mpi_proxy.fppsrc/common/m_variables_conversion.fppsrc/simulation/m_qbmm.fppsrc/simulation/m_bubbles_EL.fppsrc/pre_process/m_assign_variables.fppsrc/simulation/m_global_parameters.fppsrc/simulation/m_bubbles.fppsrc/simulation/m_bubbles_EE.fppsrc/post_process/m_mpi_proxy.fppsrc/simulation/m_start_up.fpp
📚 Learning: 2025-11-24T21:50:16.713Z
Learnt from: CR
Repo: MFlowCode/MFC PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-24T21:50:16.713Z
Learning: Applies to src/simulation/**/*.{fpp,f90} : Avoid stop/error stop inside GPU device code
Applied to files:
src/simulation/m_time_steppers.fppsrc/simulation/m_mpi_proxy.fppsrc/common/m_variables_conversion.fppsrc/simulation/m_qbmm.fppsrc/simulation/m_bubbles_EL.fppsrc/simulation/m_bubbles_EE.fppsrc/simulation/m_start_up.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: Add and update focused tests for changes; ensure no regressions in golden test outputs without clear justification
Applied to files:
tests/5CAA4E68/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/5CAA4E68/golden-metadata.txtsrc/simulation/m_mpi_proxy.fppsrc/pre_process/m_mpi_proxy.fppsrc/common/m_variables_conversion.fppsrc/simulation/m_qbmm.fppsrc/simulation/m_global_parameters.fppsrc/simulation/m_bubbles.fppsrc/simulation/m_bubbles_EE.fppsrc/post_process/m_mpi_proxy.fppsrc/simulation/m_start_up.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: 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/5CAA4E68/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: 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:
tests/5CAA4E68/golden-metadata.txtsrc/simulation/m_mpi_proxy.fppsrc/pre_process/m_mpi_proxy.fppsrc/simulation/m_bubbles.fppsrc/simulation/m_bubbles_EE.fppsrc/post_process/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: Review Fypp macros in src/<subprogram>/include/ before reviewing generated code
Applied to files:
tests/5CAA4E68/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: Applies to src/**/*.fpp : Use `.fpp` file extension for Fypp preprocessed files; CMake transpiles them to `.f90`
Applied to files:
tests/5CAA4E68/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: Fypp macros are defined in `src/<subprogram>/include/` directories where `<subprogram>` ∈ {`simulation`,`common`,`pre_process`,`post_process`}; scan these first when understanding macro usage
Applied to files:
tests/5CAA4E68/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/5CAA4E68/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: Applies to src/**/*.fpp : Start module files with Fypp include for macros: `#:include 'macros.fpp'`
Applied to files:
tests/5CAA4E68/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: Applies to src/**/*.fpp : Use fypp macro `@:ALLOCATE(var1, var2)` for device-aware allocation instead of standard Fortran `allocate`
Applied to files:
tests/5CAA4E68/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: Applies to **/*.{fpp,f90} : Use `private` declaration followed by explicit `public` exports in modules
Applied to files:
src/pre_process/m_start_up.fppsrc/simulation/m_mpi_proxy.fppsrc/common/m_derived_types.fppsrc/pre_process/m_mpi_proxy.fppsrc/common/m_helper.fppsrc/pre_process/m_assign_variables.fppsrc/simulation/m_global_parameters.fppsrc/pre_process/m_global_parameters.fppsrc/simulation/m_bubbles.fppsrc/post_process/m_mpi_proxy.fppsrc/simulation/m_start_up.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.fppsrc/pre_process/m_mpi_proxy.fppsrc/simulation/m_qbmm.fppsrc/simulation/m_global_parameters.fppsrc/simulation/m_bubbles.fppsrc/simulation/m_bubbles_EE.fppsrc/post_process/m_mpi_proxy.fppsrc/simulation/m_start_up.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} : Mark GPU-callable helpers with `$:GPU_ROUTINE(function_name='...', parallelism='[seq]')` immediately after declaration
Applied to files:
src/simulation/m_mpi_proxy.fppsrc/simulation/m_qbmm.fppsrc/simulation/m_global_parameters.fppsrc/simulation/m_bubbles.fppsrc/simulation/m_bubbles_EE.fppsrc/simulation/m_start_up.fpp
📚 Learning: 2025-11-24T21:50:46.909Z
Learnt from: CR
Repo: MFlowCode/MFC PR: 0
File: .cursor/rules/mfc-agent-rules.mdc:0-0
Timestamp: 2025-11-24T21:50:46.909Z
Learning: Applies to **/*.{fpp,f90} : Use derived types with pointers for encapsulation (e.g., `pointer, dimension(:,:,:) => null()`)
Applied to files:
src/common/m_derived_types.fppsrc/post_process/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/pre_process/m_mpi_proxy.fppsrc/simulation/m_bubbles.fppsrc/post_process/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} : Keep private helpers in the module; avoid nested procedures
Applied to files:
src/pre_process/m_mpi_proxy.fppsrc/common/m_helper.fppsrc/simulation/m_bubbles.fppsrc/post_process/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/pre_process/m_mpi_proxy.fppsrc/simulation/m_bubbles.fppsrc/post_process/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} : Each variable should have one clear purpose; do not use the same variable for multiple purposes
Applied to files:
src/pre_process/m_mpi_proxy.fpptoolchain/mfc/run/case_dicts.pysrc/common/m_helper.fppsrc/pre_process/m_assign_variables.fpp
📚 Learning: 2025-11-24T21:50:46.909Z
Learnt from: CR
Repo: MFlowCode/MFC PR: 0
File: .cursor/rules/mfc-agent-rules.mdc:0-0
Timestamp: 2025-11-24T21:50:46.909Z
Learning: Applies to **/*.{fpp,f90} : Use `wp` (working precision) parameter from `m_precision_select` instead of hardcoded precision like `real*8`
Applied to files:
src/common/m_variables_conversion.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} : Every variable must have `intent(in|out|inout)` specification and appropriate `dimension` / `allocatable` / `pointer`
Applied to files:
src/common/m_helper.fppsrc/simulation/m_global_parameters.fppsrc/simulation/m_bubbles.fppsrc/post_process/m_mpi_proxy.fppsrc/post_process/m_global_parameters.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} : 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_qbmm.fppsrc/simulation/m_global_parameters.fppsrc/pre_process/m_global_parameters.fppsrc/simulation/m_start_up.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} : Allocate large arrays with `managed` or move them into a persistent `$:GPU_ENTER_DATA(...)` region at start-up
Applied to files:
src/simulation/m_qbmm.fppsrc/simulation/m_global_parameters.fppsrc/pre_process/m_global_parameters.fppsrc/simulation/m_bubbles_EE.fppsrc/simulation/m_start_up.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} : Every argument must have explicit intent; use dimension/allocatable/pointer as appropriate
Applied to files:
src/simulation/m_global_parameters.fppsrc/simulation/m_bubbles.fppsrc/post_process/m_mpi_proxy.fppsrc/post_process/m_global_parameters.fpp
📚 Learning: 2025-11-24T21:50:46.909Z
Learnt from: CR
Repo: MFlowCode/MFC PR: 0
File: .cursor/rules/mfc-agent-rules.mdc:0-0
Timestamp: 2025-11-24T21:50:46.909Z
Learning: Applies to **/*.{fpp,f90} : Use `s_mpi_abort(<msg>)` for error termination instead of `stop`
Applied to files:
src/simulation/m_bubbles_EE.fppsrc/post_process/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} : Call s_mpi_abort(<msg>) for errors, never use stop or error stop
Applied to files:
src/simulation/m_bubbles_EE.fppsrc/post_process/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 place `stop` or `error stop` inside device code
Applied to files:
src/simulation/m_bubbles_EE.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). (14)
- GitHub Check: Github (ubuntu, mpi, debug, true)
- GitHub Check: Github (ubuntu, mpi, no-debug, true)
- GitHub Check: Github (ubuntu, no-mpi, single, no-debug, false)
- GitHub Check: Georgia Tech | Phoenix (gpu-acc)
- GitHub Check: Github (ubuntu, mpi, no-debug, false)
- GitHub Check: Github (ubuntu, mpi, debug, false)
- GitHub Check: Georgia Tech | Phoenix (cpu)
- 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: Coverage Test on CodeCov
- GitHub Check: Code Cleanliness Check
- GitHub Check: Build & Publish
🔇 Additional comments (46)
toolchain/mfc/test/case.py (1)
59-66: LGTM - Legacy parameters removed in alignment with bub_pp refactor.The removal of
Ca,Web,Re_inv,R0ref, and Lagrangian parameters (c0,rho0,T0,x0,Thost) fromBASE_CFGis consistent with the PR's objective to consolidate bubble parameters into the newbub_ppstructure. Individual test cases will now specify these viabub_pp%...entries.src/post_process/m_mpi_proxy.fpp (1)
139-146: LGTM - Scalar broadcasts are correctly implemented.All
bub_pp%members being broadcast (R0ref, p0ref, rho0ref, T0ref, ss, pv, vd, mu_l, mu_v, mu_g, gam_v, gam_g, M_v, M_g, k_v, k_g, cp_v, cp_g, R_v, R_g) are scalarreal(wp)values per the derived type definition. Using count=1 is correct.The conditional guard
if (bubbles_euler .or. bubbles_lagrange)ensures these broadcasts only occur when bubble models are active.tests/5CAA4E68/golden-metadata.txt (1)
1-7: Advisory: Golden metadata contains local development artifacts.The metadata includes developer-specific paths (e.g.,
/Users/hyeoksu/MyWork/MFC-local/MFC/bubnorm/) and indicates a "dirty" Git state. While this captures the environment where golden outputs were generated, it may affect reproducibility documentation.Consider regenerating golden files from a clean Git state if this test case is intended for CI validation.
examples/2D_whale_bubble_annulus/case.py (1)
127-140: LGTM - Bubble parameters correctly structured.The transition from legacy parameters to the new
bub_ppstructure is properly implemented:
- Fluid properties correctly compute gamma and pi_inf using the stiffened gas EOS formulation
- Bubble parameters (
R0ref,p0ref,rho0ref,pv,gam_g) are appropriately nondimensionalizedexamples/3D_lagrange_bubblescreen/case.py (2)
143-144: LGTM - Lagrangian bubble configuration flags correctly set.The addition of
"thermal": 3and"polytropic": "F"aligns with the new validator requirements forbubbles_lagrangeand the PR's standardization of non-polytropic Lagrangian bubble models.
157-164: LGTM - Reference and transport parameters correctly assigned.The bubble reference parameters (
R0ref,p0ref,rho0ref,T0ref) and transport properties (ss,pv,vd,mu_l) are correctly nondimensionalized using the reference scales.examples/3D_lagrange_shbubcollapse/case.py (1)
145-149: Confirm KM +polytropic="F"+thermal=3is the intended combination for this example.
Given this PR refactors initialization/validation, please double-check this case still exercises the intended physics path (and doesn’t silently fall back / reinterpret flags).toolchain/mfc/test/cases.py (1)
515-523:bub_ppconsolidation in Eulerian bubble tests looks consistent.
The_v/_gfields (gamma, M, k, R) appear correctly assigned in these dictionaries.Also applies to: 793-801
examples/1D_bubblescreen/case.py (1)
47-65: Ref-based nondimensionalization +bub_ppmapping looks consistent in this example.
Notablypi_inf_lis correctly scaled byp0, and acoustic magnitude/wavelength are scaled byp0/x0.Also applies to: 124-167
examples/1D_exp_bubscreen/case.py (4)
5-55: Units/nondimensional reference block is coherent here (R_uni/M_ gives J/kg/K).*
This file’sR_uni/M_v/M_gconvention is consistent and matches the intendedR_v,R_gunits.
62-67: dt/Tfinal scaling looks correct (dt = cfldxu0/cphysical).
No concerns with the revised nondimensional CFL expression and the consistentTpulse*u0/x0factor inTfinal.
126-138: p0ext/p0 patch pressure normalization is a good cleanup—verify downstream expects nondimensional “pres”.
This is the right direction; just ensure the consuming Fortran path treatspatch_icpp(* )%presas nondimensional for this example type (especially if any legacy path assumed dimensional Pa).
143-144: bub_pp + acoustic scaling is consistent with the new parameterization.
pa/p0andwavelength/x0are the right nondimensional inputs, and the minimalbub_ppset matches what this example appears to need.Also applies to: 151-158, 167-168
examples/0D_bubblecollapse_adap/case.py (3)
7-43: Refactor to named physical constants + reference scaling looks good.
The reorganized parameter block reads much clearer and aligns with the PR’s bub_pp-centric approach.
61-63: Domain/patch nondimensionalization updates are consistent (L/x0, p0ext/p0, R0ref/x0).
Nothing to flag here.Also applies to: 97-103
108-118: bub_pp mapping + adap_dt settings + fluid_pp gamma/pi_inf look aligned with the new model.
No issues spotted in these conversions.Also applies to: 124-125
src/post_process/m_start_up.fpp (2)
98-98: LGTM:bub_ppadded to namelist.This correctly integrates the new bubble parameter container into the post-process input reading.
964-966: LGTM: Unified bubble initialization path.The conditional check for
bubbles_euler .or. bubbles_lagrangeand the call tos_initialize_bubbles_model()properly centralizes bubble model initialization, replacing the previous scattered polydisperse/quadrature setup.src/simulation/m_bubbles_EL.fpp (2)
323-323: LGTM: Gas mass calculation usesR_g.The gas constant naming change from
R_ntoR_galigns with the PR's standardization of gas/vapor nomenclature.
337-338: LGTM: Heat capacity and thermal conductivity use standardized names.The changes from
cp_n→cp_gandk_nl→k_glfollow the consistent naming convention introduced in this PR.src/pre_process/m_start_up.fpp (2)
138-139: LGTM:bub_ppadded to pre-process namelist.The bubble parameter container is correctly integrated into the pre-process input handling.
775-777: LGTM: Unified bubble initialization in pre-process.Consistent with the post-process changes, this centralizes bubble model initialization when either
bubbles_eulerorbubbles_lagrangeis active.src/pre_process/m_mpi_proxy.fpp (2)
146-148: LGTM: Simplified fluid_pp broadcast.The removal of
D_vfrom the per-fluid broadcast loop is correct since bubble-related parameters are now consolidated inbub_pp.
159-166: LGTM: Consolidatedbub_ppMPI broadcasts.The new conditional block properly broadcasts all bubble parameters when
bubbles_eulerorbubbles_lagrangeis enabled. This cleanly separates bubble-specific parameters from general fluid properties.src/simulation/m_mpi_proxy.fpp (2)
191-193: LGTM: Consistent with pre-process MPI proxy.The fluid_pp broadcast simplification mirrors the pre-process changes.
197-203: LGTM:bub_ppbroadcasts in simulation MPI proxy.Consistent with the pre-process implementation, all bubble parameters are broadcast in a dedicated conditional block.
src/simulation/m_bubbles_EE.fpp (2)
274-293: LGTM: Restructured alpha-based branching.The new branching logic using
small_alfthreshold properly handles the low-alpha case by zeroing source terms, and the non-polytropic calculations are correctly placed inside theelsebranch.Note: The initialization at lines 188-191 zeros
bub_p_srcandbub_m_srcat the start, so the conditional zeroing at lines 278-281 is safe even for the polytropic case.
296-317: LGTM: Adaptive and non-adaptive time stepping paths.The restructured adaptive time stepping correctly:
- Initializes
adap_dt_stopinside the conditional block- Calls
s_advance_stepwith updated arguments includingdivu_in%sf(j, k, l)directly- Updates conservative variables directly when adapting
- Falls back to
f_rddotcomputation when not adaptingLine 316: The assignment
bub_r_src(j, k, l, q) = q_cons_vf(vs(q))%sf(j, k, l)is correct - it assigns the velocity conservative variable (nRdot) as the radius source term, which is physically appropriate for the bubble radius evolution equation.src/pre_process/m_assign_variables.fpp (1)
225-226: LGTM!The replacement of
fluid_pp(1)%pvwithbub_pp%pvaligns with the PR's goal of centralizing bubble parameters.src/common/m_derived_types.fpp (1)
354-377: Well-structured new derived type for bubble parameters.The
subgrid_bubble_physical_parameterstype is cleanly documented and consolidates bubble-related physical parameters. The field naming follows the new_g/_vconvention consistently.toolchain/mfc/run/case_dicts.py (1)
155-156: LGTM!The
fluid_ppattribute reduction is consistent across all sections and aligns with the migration of bubble-specific parameters tobub_pp.Also applies to: 421-422, 545-546
src/simulation/m_start_up.fpp (3)
158-158: LGTM!Adding
bub_ppto theuser_inputsnamelist enables reading the new bubble parameter structure from input files.
1308-1310: LGTM!The condition change from
bubbles_euler .and. nb > 1tobubbles_euler .or. bubbles_lagrangecorrectly broadens bubble model initialization to cover both Euler-Euler and Euler-Lagrange approaches, and delegates to the new centralizeds_initialize_bubbles_model().
1503-1506: Remove undefined variables from GPU_UPDATE macro at line 1506.The GPU_UPDATE list contains five undefined variables:
R_n,M_n,k_n,gamma_n, andmu_n. These variables do not exist inm_global_parametersor anywhere else in the codebase. The established naming convention uses_v(vapor) and_g(gas/mixture) suffixes, not_n. Remove these five undefined variables from the device list, or replace them with the correct corresponding variables (R_v/R_g,M_v/M_g,k_v/k_g,gam_v/gam_g,mu_v/mu_g) if they were intended.⛔ Skipped due to learnings
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 macrosLearnt 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(...)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 compilersLearnt 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} : Mark GPU-callable helpers with `$:GPU_ROUTINE(function_name='...', parallelism='[seq]')` immediately after declarationsrc/common/m_helper.fpp (2)
143-194: LGTM!The
s_initialize_bubble_vars()subroutine correctly initializes bubble variables frombub_pp, computes nondimensional numbers (Eu, Ca, Web, Re_inv, Pe_c), and delegates tos_initialize_nonpoly()for non-polytropic cases. The use off_is_defaultguards for optional parameters is appropriate.
110-141: Well-structured bubble model initialization.The
s_initialize_bubbles_model()subroutine properly organizes the initialization sequence: memory allocation, quadrature weight/node computation, and variable initialization. The conditional allocation based onpolytropicandqbmmflags is correct.examples/1D_poly_bubscreen/case.py (1)
136-143: LGTM!The bub_pp parameter normalizations are dimensionally consistent. Reference values correctly normalize to unity, and physical properties (surface tension, viscosity, vapor pressure) use standard nondimensionalization.
src/post_process/m_global_parameters.fpp (1)
204-207: LGTM!The new
bub_ppcontainer of typesubgrid_bubble_physical_parametersis correctly declared and integrates well with the existing module structure.src/pre_process/m_global_parameters.fpp (2)
508-508: LGTM!The
thermalvariable is now properly initialized todflt_intin the defaults subroutine, addressing the concern from the past review.
216-219: LGTM!The new
bub_ppcontainer declaration follows the established pattern and integrates cleanly with the existing module structure.src/simulation/m_bubbles.fpp (4)
434-435: Verifygrad_Tcalculation against physical model.A past review suggested reintroducing the
Tw(wall temperature) scaling factor in thegrad_Tcalculation. The current implementation computes a normalized temperature gradient without explicitTwscaling. Please verify this formulation is consistent with the intended heat transfer model (Ando 2010 or similar).
54-54: LGTM!The replacement of
prefwithEu(Euler number) for the driving pressure offset in the Gilmore bubble model is consistent with the PR's new normalization scheme.
732-734: LGTM!The early returns when intermediate radii become negative are a sensible numerical stability improvement. Setting
err = adap_dt_tol + 1correctly triggers step size reduction in the adaptive time-stepping algorithm.Also applies to: 749-751, 766-768
787-789: LGTM!The error correction for non-oscillating bubbles (where both initial and final velocities are approximately zero) prevents spurious error estimates from division by small numbers.
src/simulation/m_global_parameters.fpp (2)
971-971: LGTM!The assignment
gam = bub_pp%gam_gcorrectly initializes the gas gamma ratio from the bubble parameter container within the bubble-enabled code path.
731-731: LGTM!Using the named constant
dflt_adap_dt_tolinstead ofdflt_realprovides a sensible default tolerance for adaptive time stepping configuration.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/documentation/case.md (1)
744-782: Correctbubble_modeldoc: text says “1 and 2” but table defines 1–3.After introducing Rayleigh–Plesset as
bubble_model = 3(Line 748-749, 781-782), the narrative still says “by an integer of 1 and 2” (Line 780). This is user-facing and can misconfigure runs.-- `bubble_model` specified a model for spherical bubble dynamics by an integer of 1 and 2. +- `bubble_model` specifies a model for spherical bubble dynamics by an integer of 1 through 3.
🧹 Nitpick comments (1)
docs/documentation/case.md (1)
749-773: Tighten footnote/marker consistency for required-parameter notation.The requirement markers are slightly inconsistent (e.g.,
bub_pp%vdshows† ‡with an extra space at Line 755). Consider normalizing the markers (*†‡) and ensuring the requirement bullets match the table’s markers to avoid ambiguity asbub_ppexpands.Also applies to: 783-785
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
docs/documentation/case.md(2 hunks)src/common/m_derived_types.fpp(1 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_derived_types.fpp
src/**/*.fpp
📄 CodeRabbit inference engine (.cursor/rules/mfc-agent-rules.mdc)
src/**/*.fpp: Use.fppfile extension for Fypp preprocessed files; CMake transpiles them to.f90
Start module files with Fypp include for macros:#:include 'macros.fpp'
Use the fyppASSERTmacro for validating conditions:@:ASSERT(predicate, message)
Use fypp macro@:ALLOCATE(var1, var2)for device-aware allocation instead of standard Fortranallocate
Use fypp macro@:DEALLOCATE(var1, var2)for device-aware deallocation instead of standard Fortrandeallocate
Files:
src/common/m_derived_types.fpp
🧠 Learnings (2)
📚 Learning: 2025-11-24T21:50:46.909Z
Learnt from: CR
Repo: MFlowCode/MFC PR: 0
File: .cursor/rules/mfc-agent-rules.mdc:0-0
Timestamp: 2025-11-24T21:50:46.909Z
Learning: Applies to **/*.{fpp,f90} : Use `private` declaration followed by explicit `public` exports in modules
Applied to files:
src/common/m_derived_types.fpp
📚 Learning: 2025-11-24T21:50:46.909Z
Learnt from: CR
Repo: MFlowCode/MFC PR: 0
File: .cursor/rules/mfc-agent-rules.mdc:0-0
Timestamp: 2025-11-24T21:50:46.909Z
Learning: Applies to **/*.{fpp,f90} : Use derived types with pointers for encapsulation (e.g., `pointer, dimension(:,:,:) => null()`)
Applied to files:
src/common/m_derived_types.fpp
🪛 markdownlint-cli2 (0.18.1)
docs/documentation/case.md
746-746: Hard tabs
Column: 31
(MD010, no-hard-tabs)
746-746: Hard tabs
Column: 67
(MD010, no-hard-tabs)
747-747: Hard tabs
Column: 31
(MD010, no-hard-tabs)
747-747: Hard tabs
Column: 65
(MD010, no-hard-tabs)
748-748: Hard tabs
Column: 31
(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). (14)
- GitHub Check: Github (ubuntu, mpi, no-debug, true)
- GitHub Check: Github (ubuntu, mpi, debug, true)
- GitHub Check: Github (ubuntu, no-mpi, single, no-debug, false)
- GitHub Check: Github (ubuntu, mpi, debug, false)
- GitHub Check: Oak Ridge | Frontier (cpu)
- GitHub Check: Github (ubuntu, mpi, no-debug, false)
- GitHub Check: Oak Ridge | Frontier (gpu-acc)
- GitHub Check: Oak Ridge | Frontier (gpu-omp)
- GitHub Check: Georgia Tech | Phoenix (gpu-acc)
- GitHub Check: Georgia Tech | Phoenix (gpu-omp)
- GitHub Check: Georgia Tech | Phoenix (cpu)
- GitHub Check: Coverage Test on CodeCov
- GitHub Check: Code Cleanliness Check
- GitHub Check: Build & Publish
🔇 Additional comments (1)
src/common/m_derived_types.fpp (1)
354-376: LGTM! Well-structured bubble parameter container.The new
subgrid_bubble_physical_parameterstype successfully centralizes sub-grid bubble model parameters. The naming convention (_g for gas, _v for vapor) aligns with the PR's standardization effort, and the type is well-documented with appropriate inline comments. All fields usereal(wp)for precision consistency.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #1085 +/- ##
==========================================
- Coverage 44.08% 43.99% -0.09%
==========================================
Files 71 71
Lines 20332 20289 -43
Branches 1981 1982 +1
==========================================
- Hits 8963 8927 -36
+ Misses 10236 10225 -11
- Partials 1133 1137 +4 ☔ 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 · |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
src/simulation/m_time_steppers.fpp (1)
515-515: Unused local variablesdenomandnbubbleshould be removed.These variables are declared but never referenced in
s_tvd_rk. Remove them to eliminate compiler warnings and dead code.- real(wp) :: denom, nbubbletoolchain/mfc/run/case_dicts.py (1)
110-113: Extract duplicatedbub_pp%variable list into a constant.The same 20-variable list for
bub_pp%parameters is repeated identically at lines 110-113 (PRE_PROCESS), 362-365 (SIMULATION), and 514-517 (POST_PROCESS). If the variable list needs updating, all three locations must be changed in sync, risking inconsistency.Consider defining a constant:
BUB_PP_VARS = ["R0ref", "p0ref", "rho0ref", "T0ref", "ss", "pv", "vd", "mu_l", "mu_v", "mu_g", "gam_v", "gam_g", "M_v", "M_g", "k_v", "k_g", "cp_v", "cp_g", "R_v", "R_g"] # Then use in each dictionary: for var in BUB_PP_VARS: PRE_PROCESS[f"bub_pp%{var}"] = ParamType.REALdocs/documentation/case.md (1)
755-757: Hard tabs detected in table rows.Static analysis flags hard tabs (MD010) in the
bubbles_euler,bubbles_lagrange, andbubble_modeltable rows at columns 31 and 65-67. Replace tabs with spaces for consistent markdown rendering.Apply this fix to replace hard tabs with spaces:
-| `bubbles_euler` | Logical | Ensemble-averaged bubble modeling | -| `bubbles_lagrange` | Logical | Volume-averaged bubble modeling | -| `bubble_model` | Integer | [1] Gilmore; [2] Keller--Miksis; [3] Rayleigh-Plesset | +| `bubbles_euler` | Logical | Ensemble-averaged bubble modeling | +| `bubbles_lagrange` | Logical | Volume-averaged bubble modeling | +| `bubble_model` | Integer | [1] Gilmore; [2] Keller--Miksis; [3] Rayleigh-Plesset |
🧹 Nitpick comments (1)
src/pre_process/m_global_parameters.fpp (1)
263-264: Potential confusion:k_vandk_gdeclared as both scalars and arrays.Line 263 declares
k_v, k_gas allocatable arrays (real(wp), dimension(:), allocatable :: ... k_v, k_g), butbub_pp%k_vandbub_pp%k_gare scalars in the derived type. This naming collision could cause confusion. Consider renaming one set (e.g.,k_v_arrfor the array version) or documenting the distinction clearly.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
docs/documentation/case.md(3 hunks)src/common/m_derived_types.fpp(1 hunks)src/pre_process/m_global_parameters.fpp(5 hunks)src/simulation/m_mpi_proxy.fpp(1 hunks)src/simulation/m_time_steppers.fpp(1 hunks)toolchain/mfc/run/case_dicts.py(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/simulation/m_mpi_proxy.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_time_steppers.fppsrc/common/m_derived_types.fppsrc/pre_process/m_global_parameters.fpp
src/simulation/**/*.{fpp,f90}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
src/simulation/**/*.{fpp,f90}: Wrap tight GPU loops with !$acc parallel loop gang vector default(present) reduction(...); add collapse(n) when safe; declare loop-local variables with private(...)
Allocate large GPU arrays with managed memory or move them into persistent !$acc enter data regions at start-up
Avoid stop/error stop inside GPU device code
Ensure GPU code compiles with Cray ftn, NVIDIA nvfortran, GNU gfortran, and Intel ifx/ifort compilers
src/simulation/**/*.{fpp,f90}: Mark GPU-callable helpers with$:GPU_ROUTINE(function_name='...', parallelism='[seq]')immediately after declaration
Do not use OpenACC or OpenMP directives directly; use Fypp macros fromsrc/common/include/parallel_macros.fppinstead
Wrap tight loops with$:GPU_PARALLEL_FOR(private='[...]', copy='[...]')macro; addcollapse=nfor safe nested loop merging
Declare loop-local variables withprivate='[...]'in GPU parallel loop macros
Allocate large arrays withmanagedor move them into a persistent$:GPU_ENTER_DATA(...)region at start-up
Do not placestoporerror stopinside device code
Files:
src/simulation/m_time_steppers.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_time_steppers.fppsrc/common/m_derived_types.fppsrc/pre_process/m_global_parameters.fpp
🧠 Learnings (7)
📚 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_time_steppers.fpp
📚 Learning: 2025-11-24T21:50:16.713Z
Learnt from: CR
Repo: MFlowCode/MFC PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-24T21:50:16.713Z
Learning: Applies to src/simulation/**/*.{fpp,f90} : Wrap tight GPU loops with !$acc parallel loop gang vector default(present) reduction(...); add collapse(n) when safe; declare loop-local variables with private(...)
Applied to files:
src/simulation/m_time_steppers.fpp
📚 Learning: 2025-11-24T21:50:16.713Z
Learnt from: CR
Repo: MFlowCode/MFC PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-24T21:50:16.713Z
Learning: Applies to src/simulation/**/*.{fpp,f90} : Avoid stop/error stop inside GPU device code
Applied to files:
src/simulation/m_time_steppers.fpp
📚 Learning: 2025-11-24T21:50:46.909Z
Learnt from: CR
Repo: MFlowCode/MFC PR: 0
File: .cursor/rules/mfc-agent-rules.mdc:0-0
Timestamp: 2025-11-24T21:50:46.909Z
Learning: Applies to **/*.{fpp,f90} : Use `private` declaration followed by explicit `public` exports in modules
Applied to files:
src/common/m_derived_types.fppsrc/pre_process/m_global_parameters.fpp
📚 Learning: 2025-11-24T21:50:46.909Z
Learnt from: CR
Repo: MFlowCode/MFC PR: 0
File: .cursor/rules/mfc-agent-rules.mdc:0-0
Timestamp: 2025-11-24T21:50:46.909Z
Learning: Applies to 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/pre_process/m_global_parameters.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} : Allocate large GPU arrays with managed memory or move them into persistent !$acc enter data regions at start-up
Applied to files:
src/pre_process/m_global_parameters.fpp
📚 Learning: 2025-11-24T21:50:46.909Z
Learnt from: CR
Repo: MFlowCode/MFC PR: 0
File: .cursor/rules/mfc-agent-rules.mdc:0-0
Timestamp: 2025-11-24T21:50:46.909Z
Learning: Applies to **/*.{fpp,f90} : Each variable should have one clear purpose; do not use the same variable for multiple purposes
Applied to files:
toolchain/mfc/run/case_dicts.py
🧬 Code graph analysis (3)
src/common/m_derived_types.fpp (1)
toolchain/mfc/test/cases.py (1)
alter_viscosity(786-835)
docs/documentation/case.md (1)
toolchain/mfc/test/cases.py (1)
alter_viscosity(786-835)
toolchain/mfc/run/case_dicts.py (1)
toolchain/mfc/case.py (1)
Case(19-292)
🪛 markdownlint-cli2 (0.18.1)
docs/documentation/case.md
755-755: Hard tabs
Column: 31
(MD010, no-hard-tabs)
755-755: Hard tabs
Column: 67
(MD010, no-hard-tabs)
756-756: Hard tabs
Column: 31
(MD010, no-hard-tabs)
756-756: Hard tabs
Column: 65
(MD010, no-hard-tabs)
757-757: Hard tabs
Column: 31
(MD010, no-hard-tabs)
803-803: Hard tabs
Column: 29
(MD010, no-hard-tabs)
804-804: Hard tabs
Column: 8
(MD010, no-hard-tabs)
804-804: Hard tabs
Column: 27
(MD010, no-hard-tabs)
805-805: Hard tabs
Column: 16
(MD010, no-hard-tabs)
805-805: Hard tabs
Column: 27
(MD010, no-hard-tabs)
805-805: Hard tabs
Column: 30
(MD010, no-hard-tabs)
806-806: Hard tabs
Column: 10
(MD010, no-hard-tabs)
806-806: Hard tabs
Column: 30
(MD010, no-hard-tabs)
806-806: Hard tabs
Column: 32
(MD010, no-hard-tabs)
807-807: Hard tabs
Column: 32
(MD010, no-hard-tabs)
808-808: Hard tabs
Column: 10
(MD010, no-hard-tabs)
808-808: Hard tabs
Column: 27
(MD010, no-hard-tabs)
808-808: Hard tabs
Column: 30
(MD010, no-hard-tabs)
809-809: Hard tabs
Column: 10
(MD010, no-hard-tabs)
809-809: Hard tabs
Column: 27
(MD010, no-hard-tabs)
809-809: Hard tabs
Column: 30
(MD010, no-hard-tabs)
810-810: Hard tabs
Column: 10
(MD010, no-hard-tabs)
810-810: Hard tabs
Column: 27
(MD010, no-hard-tabs)
810-810: Hard tabs
Column: 30
(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). (16)
- GitHub Check: Coverage Test on CodeCov
- GitHub Check: Github (ubuntu, mpi, no-debug, false)
- GitHub Check: Georgia Tech | Phoenix (cpu)
- GitHub Check: Github (ubuntu, no-mpi, single, no-debug, false)
- GitHub Check: Github (macos, mpi, debug, false)
- GitHub Check: Georgia Tech | Phoenix (gpu-acc)
- GitHub Check: Github (ubuntu, mpi, debug, true)
- GitHub Check: Github (ubuntu, mpi, debug, false)
- GitHub Check: Github (ubuntu, mpi, no-debug, true)
- GitHub Check: Github (macos, mpi, no-debug, false)
- GitHub Check: Georgia Tech | Phoenix (gpu-omp)
- GitHub Check: Oak Ridge | Frontier (gpu-acc)
- GitHub Check: Oak Ridge | Frontier (gpu-omp)
- GitHub Check: Oak Ridge | Frontier (cpu)
- GitHub Check: Code Cleanliness Check
- GitHub Check: Build & Publish
🔇 Additional comments (7)
src/common/m_derived_types.fpp (2)
356-378: Well-structured new derived type for subgrid bubble parameters.The
subgrid_bubble_physical_parameterstype cleanly consolidates bubble-related physical parameters that were previously scattered. The Doxygen-style comments clearly document each field's purpose (host vs gas, liquid/vapor/gas states). This improves maintainability and aligns with the PR objective of centralizing bubble parameter handling.
346-354: Slimmedphysical_parameterstype is appropriate.The removal of bubble-specific fields (
mul0,ss,pv,gamma_v,M_v,mu_v,k_v,cp_v,D_v) fromphysical_parametersis correct since these are now consolidated insubgrid_bubble_physical_parameters. The remaining fields (gamma,pi_inf,Re,cv,qv,qvp,G) are general fluid properties that belong here.src/pre_process/m_global_parameters.fpp (2)
508-508:thermalis properly initialized.The
thermalvariable is correctly initialized todflt_intin the default values subroutine. This addresses the uninitialized variable concern.
610-611: Delete this comment — the initialization is correct.
k_vandk_gare allocatable arrays (declared at line 263), not scalar variables. The initialization at lines 610–611 is correct: onlybub_pp%k_vandbub_pp%k_gshould be initialized here. The pattern at lines 596–609 and 612–615 applies to scalar variables likeR0ref,p0ref, etc., which have both member and standalone forms. Arrays likek_vandk_gdo not have scalar counterparts and should not be initialized as scalars.Likely an incorrect or invalid review comment.
toolchain/mfc/run/case_dicts.py (1)
155-156: Approve: Streamlinedfluid_ppattributes.The
fluid_ppattributes are correctly reduced to["gamma", "pi_inf", "G", "cv", "qv", "qvp"], removing bubble-specific parameters that are now inbub_pp. This aligns with the type changes inm_derived_types.fpp.docs/documentation/case.md (2)
753-778: Comprehensive documentation forbub_ppparameters.The new parameter table clearly documents all
bub_pp%fields with appropriate footnote markers indicating which parameters are required for EE vs EL models, and polytropic vs non-polytropic cases. The descriptions are clear and consistent with the type definition inm_derived_types.fpp.
792-793: Good addition ofbub_ppdescription.The new description clearly states that
bub_ppspecifies simulation parameters for both EE and EL bubble models, providing helpful context for users.
|
@sbryngelson yeah, it passed this time! |
|
/improve |
There was a problem hiding this comment.
Pull request overview
This PR performs a comprehensive refactoring of sub-grid bubble models (Euler-Euler and Euler-Lagrange) to eliminate redundancy and improve consistency. The key architectural change is the introduction of a unified bub_pp (bubble physical parameters) structure that centralizes all bubble-related parameters previously scattered across fluid_pp% and other locations. The PR also standardizes nomenclature by replacing subscript _n (originally for gas) with _g throughout the codebase, and replaces R0ref with Eu (Euler number) as the primary nondimensional parameter.
Key changes:
- Consolidated EE and EL bubble initialization into a single
s_initialize_bubbles_model()subroutine - Introduced
subgrid_bubble_physical_parametersderived type to organize all bubble parameters - Updated all example cases and test configurations to use the new
bub_pp%structure
Reviewed changes
Copilot reviewed 73 out of 74 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
src/common/m_derived_types.fpp |
Defines new subgrid_bubble_physical_parameters type with 18 bubble-related fields |
src/common/m_helper.fpp |
Creates unified s_initialize_bubbles_model() and s_initialize_bubble_vars() subroutines |
src/*/m_global_parameters.fpp |
Adds bub_pp structure declaration and removes bubble params from fluid_pp |
src/simulation/m_bubbles.fpp |
Updates nomenclature (_n→_g), replaces pref with Eu, refactors adaptive time-stepping |
src/simulation/m_bubbles_EE.fpp |
Restructures source computation with improved validation |
src/simulation/m_bubbles_EL.fpp |
Removes s_start_lagrange_inputs(), simplifies initialization |
src/*/m_mpi_proxy.fpp |
Updates MPI broadcasts for new bub_pp structure |
toolchain/mfc/run/case_dicts.py |
Adds bub_pp% parameters, removes old bubble params from fluid_pp()% |
toolchain/mfc/case_validator.py |
Updates validation logic for Lagrangian bubbles |
examples/*/case.py |
Updates 13+ example cases with new bub_pp% structure |
Co-authored-by: qodo-code-review[bot] <151058649+qodo-code-review[bot]@users.noreply.github.com>
|
CodeAnt AI is running Incremental review Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (5)
docs/documentation/case.md (2)
753-781: Remove hard tabs and standardize †‡ marker in sub-grid bubble parameter table.markdownlint still reports MD010 hard tabs on the header rows; also the †‡ marker for
bub_pp%vdis inconsistent with nearby markers.Suggested patch:
-| `bubbles_euler` | Logical | Ensemble-averaged bubble modeling | -| `bubbles_lagrange` | Logical | Volume-averaged bubble modeling | -| `bubble_model` | Integer | [1] Gilmore; [2] Keller--Miksis; [3] Rayleigh-Plesset | +| `bubbles_euler` | Logical | Ensemble-averaged bubble modeling | +| `bubbles_lagrange` | Logical | Volume-averaged bubble modeling | +| `bubble_model` | Integer | [1] Gilmore; [2] Keller--Miksis; [3] Rayleigh-Plesset | @@ -| `bub_pp%vd`† ‡ | Real | Vapor diffusivity in gas (bubble) | +| `bub_pp%vd`†‡ | Real | Vapor diffusivity in gas (bubble) |
798-810: Fix remaining hard tabs and small wording issues in ensemble-averaged bubble model table.These rows still contain hard tabs (MD010) and a minor wording glitch in the
qbmmdescription.Suggested patch:
-| `bubbles_euler` | Logical | Ensemble-averaged bubble modeling | +| `bubbles_euler` | Logical | Ensemble-averaged bubble modeling | @@ -| `thermal` | Integer | Thermal model: [1] Adiabatic; [2] Isothermal; [3] Transfer | +| `thermal` | Integer | Thermal model: [1] Adiabatic; [2] Isothermal; [3] Transfer | @@ -| `poly_sigma` | Real | Standard deviation for probability density function of polydisperse bubble populations | -| `qbmm` | Logical | Quadrature by method of moments | -| `dist_type` | Integer | Joint probability density function for bubble radius and velocity (only for ``qbmm = 'T'``) | -| `sigR` | Real | Standard deviation for the probability density function of bubble radius (only for ``qbmm = 'T'``) (EE)| -| `sigV` | Real | Standard deviation for the probability density function of bubble velocity (only for ``qbmm = 'T'``) (EE)| -| `rhoRV` | Real | Correlation coefficient for the joint probability density function of bubble radius and velocity (only for ``qbmm = 'T'``) | +| `poly_sigma` | Real | Standard deviation for probability density function of polydisperse bubble populations | +| `qbmm` | Logical | Quadrature by method of moments | +| `dist_type` | Integer | Joint probability density function for bubble radius and velocity (only for ``qbmm = 'T'``) | +| `sigR` | Real | Standard deviation for the probability density function of bubble radius (only for ``qbmm = 'T'``) (EE)| +| `sigV` | Real | Standard deviation for the probability density function of bubble velocity (only for ``qbmm = 'T'``) (EE)| +| `rhoRV` | Real | Correlation coefficient for the joint probability density function of bubble radius and velocity (only for ``qbmm = 'T'``) |src/simulation/m_bubbles.fpp (3)
434-435: Verify temperature gradient scaling factor.A previous review indicated that the
Tw(wall temperature) scaling factor may be missing from thegrad_Tcalculation. The current formula computes a dimensionless temperature ratio, but it's unclear whether dimensional consistency requires theTwfactor. Please verify the physical correctness of this expression against the governing equations.If
Twscaling is required, apply this diff:- grad_T = -Re_trans_T(iR0)*((fpb/pb0(iR0))*(fR/R0(iR0))**3 & - *(mass_g0(iR0) + mass_v0(iR0))/(mass_g0(iR0) + fmass_v) - 1._wp) + grad_T = -Re_trans_T(iR0)*Tw*((fpb/pb0(iR0))*(fR/R0(iR0))**3 & + *(mass_g0(iR0) + mass_v0(iR0))/(mass_g0(iR0) + fmass_v) - 1._wp)
360-361: Make optional arguments required or add present() guards.The optional dummy arguments
fmass_g,fbeta_c,fR_m, andfgamma_mare used unconditionally on lines 372, 376–377, 380, and 382 withoutpresent()checks. This can lead to undefined behavior if call sites omit these arguments. Since all current call sites (e.g., line 816) pass these arguments, the safest fix is to make them required by removing theoptionalattribute.Apply this diff to make the arguments required:
- real(wp), intent(in), optional :: fmass_g, fbeta_c - real(wp), intent(out), optional :: fR_m, fgamma_m + real(wp), intent(in) :: fmass_g, fbeta_c + real(wp), intent(out) :: fR_m, fgamma_m
376-376: Normalize the mixture gas constant by total mass.The mixture gas constant
fR_mis computed as a sum of mass-weighted constants but is not normalized by total mass. This produces an incorrect value scale. The formula should compute a mass-weighted average by dividing by(fmass_g + fmass_v).Apply this diff to fix the normalization:
- fR_m = (fmass_g*R_g + fmass_v*R_v) + fR_m = (fmass_g*R_g + fmass_v*R_v)/(fmass_g + fmass_v)
🧹 Nitpick comments (2)
examples/0D_bubblecollapse_adap/case.py (1)
14-24: Unused vapor and air properties.The variables
gam_v,M_v,mu_v,k_v,M_g,mu_g, andk_gare defined but never used in the configuration dictionary. Onlygam_gis used at line 114. Since this case usespolytropic: "T", these non-polytropic parameters aren't needed.Consider removing the unused variables to reduce clutter, or add a comment indicating they're kept for reference/future non-polytropic configurations.
src/simulation/m_bubbles.fpp (1)
784-786: Consider extracting the threshold as a named constant.The hardcoded threshold
1.e-12_wpfor detecting non-oscillating bubbles (line 784) would be more maintainable as a named module-level constant (e.g.,velocity_tolerance). This would improve code clarity and make it easier to adjust the threshold if needed.Example refactor:
+ real(wp), parameter :: velocity_tolerance = 1.e-12_wp ... - if (max(abs(myV_tmp(1)), abs(myV_tmp(4))) < 1.e-12_wp) then + if (max(abs(myV_tmp(1)), abs(myV_tmp(4))) < velocity_tolerance) then err_V = 0._wp end if
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
docs/documentation/case.md(1 hunks)examples/0D_bubblecollapse_adap/case.py(3 hunks)src/simulation/m_bubbles.fpp(20 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{fpp,f90}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{fpp,f90}: Use 2-space indentation; continuation lines align beneath &
Use lower-case keywords and intrinsics (do, end subroutine, etc.)
Name modules with m_ pattern (e.g., m_transport)
Name public subroutines with s_ pattern (e.g., s_compute_flux)
Name public functions with f_ pattern
Keep subroutine size ≤ 500 lines, helper subroutines ≤ 150 lines, functions ≤ 100 lines, files ≤ 1000 lines
Limit routine arguments to ≤ 6; use derived-type params struct if more are needed
Forbid goto statements (except in legacy code), COMMON blocks, and save globals
Every argument must have explicit intent; use dimension/allocatable/pointer as appropriate
Call s_mpi_abort() for errors, never use stop or error stop
**/*.{fpp,f90}: Indent 2 spaces; continuation lines align under&
Use lower-case keywords and intrinsics (do,end subroutine, etc.)
Name modules withm_<feature>prefix (e.g.,m_transport)
Name public subroutines ass_<verb>_<noun>(e.g.,s_compute_flux) and functions asf_<verb>_<noun>
Keep private helpers in the module; avoid nested procedures
Enforce size limits: subroutine ≤ 500 lines, helper ≤ 150, function ≤ 100, module/file ≤ 1000
Limit subroutines to ≤ 6 arguments; otherwise pass a derived-type 'params' struct
Avoidgotostatements (except unavoidable legacy); avoid global state (COMMON,save)
Every variable must haveintent(in|out|inout)specification and appropriatedimension/allocatable/pointer
Uses_mpi_abort(<msg>)for error termination instead ofstop
Use!>style documentation for header comments; follow Doxygen Fortran format with!! @paramand!! @returntags
Useimplicit nonestatement in all modules
Useprivatedeclaration followed by explicitpublicexports in modules
Use derived types with pointers for encapsulation (e.g.,pointer, dimension(:,:,:) => null())
Usepureandelementalattributes for side-effect-free functions; combine them for array ...
Files:
src/simulation/m_bubbles.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_bubbles.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_bubbles.fpp
🧠 Learnings (9)
📚 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_bubbles.fpp
📚 Learning: 2025-11-24T21:50:16.713Z
Learnt from: CR
Repo: MFlowCode/MFC PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-24T21:50:16.713Z
Learning: Applies to src/simulation/**/*.{fpp,f90} : Wrap tight GPU loops with !$acc parallel loop gang vector default(present) reduction(...); add collapse(n) when safe; declare loop-local variables with private(...)
Applied to files:
src/simulation/m_bubbles.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_bubbles.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} : Every argument must have explicit intent; use dimension/allocatable/pointer as appropriate
Applied to files:
src/simulation/m_bubbles.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} : Every variable must have `intent(in|out|inout)` specification and appropriate `dimension` / `allocatable` / `pointer`
Applied to files:
src/simulation/m_bubbles.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_bubbles.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_bubbles.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} : Mark GPU-callable helpers with `$:GPU_ROUTINE(function_name='...', parallelism='[seq]')` immediately after declaration
Applied to files:
src/simulation/m_bubbles.fpp
📚 Learning: 2025-11-24T21:50:46.909Z
Learnt from: CR
Repo: MFlowCode/MFC PR: 0
File: .cursor/rules/mfc-agent-rules.mdc:0-0
Timestamp: 2025-11-24T21:50:46.909Z
Learning: Applies to **/*.{fpp,f90} : Use `wp` (working precision) parameter from `m_precision_select` instead of hardcoded precision like `real*8`
Applied to files:
src/simulation/m_bubbles.fpp
🧬 Code graph analysis (1)
examples/0D_bubblecollapse_adap/case.py (1)
toolchain/mfc/test/cases.py (1)
alter_viscosity(786-835)
🪛 markdownlint-cli2 (0.18.1)
docs/documentation/case.md
755-755: Hard tabs
Column: 31
(MD010, no-hard-tabs)
755-755: Hard tabs
Column: 67
(MD010, no-hard-tabs)
756-756: Hard tabs
Column: 31
(MD010, no-hard-tabs)
756-756: Hard tabs
Column: 65
(MD010, no-hard-tabs)
757-757: Hard tabs
Column: 31
(MD010, no-hard-tabs)
800-800: Hard tabs
Column: 66
(MD010, no-hard-tabs)
802-802: Hard tabs
Column: 13
(MD010, no-hard-tabs)
805-805: Hard tabs
Column: 28
(MD010, no-hard-tabs)
805-805: Hard tabs
Column: 32
(MD010, no-hard-tabs)
806-806: Hard tabs
Column: 32
(MD010, no-hard-tabs)
807-807: Hard tabs
Column: 32
(MD010, no-hard-tabs)
808-808: Hard tabs
Column: 32
(MD010, no-hard-tabs)
809-809: Hard tabs
Column: 32
(MD010, no-hard-tabs)
810-810: Hard tabs
Column: 32
(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). (8)
- 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: Github (ubuntu, no-mpi, single, no-debug, false)
- GitHub Check: Github (ubuntu, mpi, debug, false)
- GitHub Check: Code Cleanliness Check
- GitHub Check: Coverage Test on CodeCov
- GitHub Check: Build & Publish
🔇 Additional comments (11)
docs/documentation/case.md (1)
789-792:bubble_model/bub_ppdescriptions look consistent and clear.The mapping of
bubble_model = 1,2,3to Gilmore / Keller–Miksis / Rayleigh–Plesset and the briefbub_ppdescription match the new centralized-parameter design.examples/0D_bubblecollapse_adap/case.py (1)
54-128: LGTM!The configuration dictionary is well-structured with consistent nondimensionalization. The
dtissue from past reviews has been properly resolved—the computed value is now used at line 66. Thebub_ppparameters are correctly scaled, and the fluid properties follow the stiffened gas EOS formulation.src/simulation/m_bubbles.fpp (9)
54-54: LGTM: Euler number integration.The replacement of
prefwithEuin the Gilmore branch (line 54) and the addition ofEuin the Keller-Miksis polytropic branch (line 267) correctly implements the PR's objective to use Euler number as the primary nondimensional parameter for bubble pressure calculations.Also applies to: 267-267
332-336: LGTM: Gas nomenclature standardization.The systematic renaming from
_n(subscript n) to_g(subscript g) for gas-related properties (R_g,M_g,k_g,phi_gv,phi_vg) correctly implements the PR's standardized nomenclature for gas-related variables.
468-468: LGTM: Consistent fmass_g parameter propagation.The addition of
fmass_g(gas mass) to the signatures ofs_advance_step,s_advance_substep, ands_advance_ELcorrectly implements the PR's gas-centric parameter handling. The parameter flows consistently through the call chain:s_advance_step→s_advance_substep→s_advance_EL→s_vflux.Also applies to: 477-477, 694-694, 704-704, 807-807, 810-810
482-482: LGTM: Clearer time step initialization.The refactored time step initialization (declaring
h0separately at line 482, retrieving it froms_initial_substep_hat line 489, and explicitly assigningh = h0at line 490) makes the initial time step selection more explicit and easier to follow.Also applies to: 489-490
513-516: LGTM: Efficient early-exit for tolerance violations.The new early-exit checks (lines 513–516, 525–528, 539–542) improve efficiency by immediately shrinking the time step and retrying when intermediate error estimates exceed
adap_dt_tol, rather than continuing with computations that will ultimately be rejected.Also applies to: 525-528, 539-542
554-555: LGTM: Enhanced acceptance criteria (after syntax fix).The addition of
err(4)anderr(5)checks to the acceptance criteria (lines 554–555) provides more rigorous error control by also validating relative differences between one-step and two-half-step solutions for both radius and velocity. This is a good practice for adaptive time stepping.Note: This depends on fixing the syntax errors on lines 544–545.
729-731: LGTM: Negative radius validation.The new negative radius checks at each Runge-Kutta stage (lines 729–731, 746–748, 763–765) correctly implement the PR's stated objective to add validation checks. Setting
err = adap_dt_tol + 1._wpand returning immediately prevents further computation with unphysical bubble states and forces a smaller time step on retry.Also applies to: 746-748, 763-765
720-720: LGTM: Complete parameter propagation at call sites.All call sites to
s_advance_EL(lines 720, 737, 754, 771) ands_vflux(line 816) correctly pass the newfmass_gparameter, completing the parameter propagation chain for gas-centric bubble modeling.Also applies to: 737-737, 754-754, 771-771, 816-816
372-390: LGTM: Correct dual-model gas mass handling.The
s_vfluxsubroutine correctly handles gas mass differently for Lagrangian bubbles (using the passedfmass_gparameter at lines 372, 380, 382) versus ensemble-averaged bubbles (using the indexed arraymass_g0(iR0)at line 388). The documentation at line 347 appropriately clarifies thatiR0serves as either a bubble size index (EE) or bubble identifier (EL).
can you look at these @hyeoksu-lee ? they seem like possible bugs (maybe not all of them) |
some of these seeem reasonably worth looking at too, as a double check |
The first two (Swapped gas/vapor properties / Swapped properties) and the last one (Possible Bug (swapped masses)) are requiring fixes. As noted in the PR description, I believe the current master has the swapped properties bug in EL bubble model. Therefore, I made them swapped to make this PR passes CI test, and I am planning to fix this bug in the next PR. This is because the I don't want to fix too many things in this PR. I can fix this in this PR if you think it's more appropriate. The third and fourth one (Units/ Gas constant / Simulation tilmestep/count mismatch) are just following what the current master is doing.. |
|
Make a GH Issue for this one The first two (Swapped gas/vapor properties / Swapped properties) and the last one (Possible Bug (swapped masses)) are requiring fixes. As noted in the PR description, I believe the current master has the swapped properties bug in EL bubble model. Therefore, I made them swapped to make this PR passes CI test, and I am planning to fix this bug in the next PR. This is because the I don't want to fix too many things in this PR. I can fix this in this PR if you think it's more appropriate. and I'm merging |
For the first one regarding EL bubbles, what I did is just replacing The second one should be fine. For the third one, |
|
sounds good! will merge soon. oh and for the EL bubbles issue please tag @wilfonba he will know what's going on (also tag @dgvacarevelo it might be his test) |
Ok I'll do |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
examples/0D_bubblecollapse_adap/case.py (1)
15-24: Several vapor and gas properties are defined but unused.The following properties are defined but not referenced in the configuration dictionary:
gam_v,M_v,mu_v,k_v,M_g,mu_g,k_g. Onlygam_gis used (line 114).If these are intended for documentation purposes or future extensions, consider adding a comment to clarify. Otherwise, they can be removed to reduce clutter.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
examples/0D_bubblecollapse_adap/case.py(3 hunks)src/simulation/m_bubbles.fpp(20 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{fpp,f90}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{fpp,f90}: Use 2-space indentation; continuation lines align beneath &
Use lower-case keywords and intrinsics (do, end subroutine, etc.)
Name modules with m_ pattern (e.g., m_transport)
Name public subroutines with s_ pattern (e.g., s_compute_flux)
Name public functions with f_ pattern
Keep subroutine size ≤ 500 lines, helper subroutines ≤ 150 lines, functions ≤ 100 lines, files ≤ 1000 lines
Limit routine arguments to ≤ 6; use derived-type params struct if more are needed
Forbid goto statements (except in legacy code), COMMON blocks, and save globals
Every argument must have explicit intent; use dimension/allocatable/pointer as appropriate
Call s_mpi_abort() for errors, never use stop or error stop
**/*.{fpp,f90}: Indent 2 spaces; continuation lines align under&
Use lower-case keywords and intrinsics (do,end subroutine, etc.)
Name modules withm_<feature>prefix (e.g.,m_transport)
Name public subroutines ass_<verb>_<noun>(e.g.,s_compute_flux) and functions asf_<verb>_<noun>
Keep private helpers in the module; avoid nested procedures
Enforce size limits: subroutine ≤ 500 lines, helper ≤ 150, function ≤ 100, module/file ≤ 1000
Limit subroutines to ≤ 6 arguments; otherwise pass a derived-type 'params' struct
Avoidgotostatements (except unavoidable legacy); avoid global state (COMMON,save)
Every variable must haveintent(in|out|inout)specification and appropriatedimension/allocatable/pointer
Uses_mpi_abort(<msg>)for error termination instead ofstop
Use!>style documentation for header comments; follow Doxygen Fortran format with!! @paramand!! @returntags
Useimplicit nonestatement in all modules
Useprivatedeclaration followed by explicitpublicexports in modules
Use derived types with pointers for encapsulation (e.g.,pointer, dimension(:,:,:) => null())
Usepureandelementalattributes for side-effect-free functions; combine them for array ...
Files:
src/simulation/m_bubbles.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_bubbles.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_bubbles.fpp
🧠 Learnings (12)
📚 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_bubbles.fpp
📚 Learning: 2025-11-24T21:50:16.713Z
Learnt from: CR
Repo: MFlowCode/MFC PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-24T21:50:16.713Z
Learning: Applies to src/simulation/**/*.{fpp,f90} : Wrap tight GPU loops with !$acc parallel loop gang vector default(present) reduction(...); add collapse(n) when safe; declare loop-local variables with private(...)
Applied to files:
src/simulation/m_bubbles.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_bubbles.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_bubbles.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} : Mark GPU-callable helpers with `$:GPU_ROUTINE(function_name='...', parallelism='[seq]')` immediately after declaration
Applied to files:
src/simulation/m_bubbles.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_bubbles.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} : Every argument must have explicit intent; use dimension/allocatable/pointer as appropriate
Applied to files:
src/simulation/m_bubbles.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} : Every variable must have `intent(in|out|inout)` specification and appropriate `dimension` / `allocatable` / `pointer`
Applied to files:
src/simulation/m_bubbles.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_bubbles.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_bubbles.fpp
📚 Learning: 2025-11-24T21:50:46.909Z
Learnt from: CR
Repo: MFlowCode/MFC PR: 0
File: .cursor/rules/mfc-agent-rules.mdc:0-0
Timestamp: 2025-11-24T21:50:46.909Z
Learning: Applies to **/*.{fpp,f90} : Use `wp` (working precision) parameter from `m_precision_select` instead of hardcoded precision like `real*8`
Applied to files:
src/simulation/m_bubbles.fpp
📚 Learning: 2025-11-24T21:50:16.713Z
Learnt from: CR
Repo: MFlowCode/MFC PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-24T21:50:16.713Z
Learning: Applies to src/simulation/**/*.{fpp,f90} : Avoid stop/error stop inside GPU device code
Applied to files:
src/simulation/m_bubbles.fpp
🧬 Code graph analysis (1)
examples/0D_bubblecollapse_adap/case.py (1)
toolchain/mfc/test/cases.py (1)
alter_viscosity(786-835)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: Github (ubuntu, 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, no-debug, false)
- GitHub Check: Github (ubuntu, mpi, no-debug, true)
- GitHub Check: Github (ubuntu, mpi, debug, true)
- GitHub Check: Github (macos, mpi, debug, false)
- GitHub Check: Code Cleanliness Check
- GitHub Check: Coverage Test on CodeCov
- GitHub Check: Build & Publish
🔇 Additional comments (7)
examples/0D_bubblecollapse_adap/case.py (1)
108-125: LGTM! Configuration properly uses bub_pp structure.The bubble parameter configuration correctly uses the new
bub_ppstructure with appropriate nondimensionalization, and the adaptive time-stepping parameters are properly configured. The fluid parameters are correctly computed from the liquid properties.src/simulation/m_bubbles.fpp (6)
54-54: LGTM! Euler number (Eu) correctly replaces previous normalization.The use of
Eufor bubble pressure nondimensionalization in both Gilmore (line 54) and Keller-Miksis (line 267) models is consistent with the PR's objective to standardize on Eu-based scaling.Also applies to: 267-267
332-338: Naming updated consistently from gas subscript _n to _g.The bubble wall property calculations now use
R_g,M_g, andk_ginstead of the previousR_n,M_n, andk_n, aligning with the PR's nomenclature standardization.
434-440: Naming updated consistently to use mass_g0 and gam_m.The internal bubble pressure derivative calculations now use
mass_g0(instead ofmass_n0) andgam_mnomenclature, consistent with the PR's standardization efforts.
482-490: LGTM! Adaptive time-stepping initialization and error handling improved.The explicit
h0initialization (lines 482-490) clarifies the initial step size computation, and the use ofsgm_epsin relative error calculations (lines 544-545) with special handling for non-oscillating bubbles (line 546) enhances numerical stability.Also applies to: 544-546
729-731: LGTM! Early-exit guards prevent negative radius continuation.The early-exit conditions when bubble radius becomes negative at any substep stage (lines 729-731, 746-748, 763-765) properly trigger step-size reduction by setting
err > adap_dt_tol, preventing unphysical states from propagating.Also applies to: 746-748, 763-765
806-820: LGTM! Lagrangian bubble advancement correctly propagates fmass_g.The
s_advance_ELsubroutine signature and implementation correctly incorporate thefmass_gparameter, passing it through tos_vfluxfor proper mixture property calculations.
|
CodeAnt AI is running Incremental review Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
src/simulation/m_bubbles.fpp (2)
360-361: Unguarded optional parameters can cause undefined behavior.The optional parameters
fmass_g,fbeta_c,fR_m, andfgamma_mare read or written at lines 372–386 withoutpresent()guards. If a caller omits these arguments, the routine will produce undefined behavior (reading uninitialized values or writing to absent dummies).While current callers (line 816) do pass these arguments, the optional declaration without guards is fragile and risky for future maintenance.
Recommended fix: Either remove the
optionalattribute (making them required) or addpresent()guards before each use:- real(wp), intent(in), optional :: fmass_g, fbeta_c - real(wp), intent(out), optional :: fR_m, fgamma_m + real(wp), intent(in) :: fmass_g, fbeta_c + real(wp), intent(out) :: fR_m, fgamma_mAlternatively, if they must remain optional, add guards:
if (present(fmass_g) .and. present(fR_m) .and. present(fgamma_m)) then ! ... existing logic at lines 372-386 end ifBased on past review comments.
376-376: Missing normalization in mixture gas constant calculation.The mixture gas constant
fR_mis computed as a sum of mass-weighted constants but is not normalized by total mass. This produces an incorrectly scaled value with units of[mass × specific_gas_constant]instead of[specific_gas_constant].Downstream usage (lines 427, 430, 817) expects
fR_mto be the mixture-averaged specific gas constant, so this error will break thermodynamic calculations for Lagrangian bubbles.Apply this diff to compute the correct mass-weighted average:
- fR_m = (fmass_g*R_g + fmass_v*R_v) + fR_m = (fmass_g*R_g + fmass_v*R_v)/(fmass_g + fmass_v)Based on past review comments.
🧹 Nitpick comments (3)
src/simulation/m_bubbles.fpp (3)
544-547: Inconsistent division-by-zero protection in relative error calculations.Line 545 guards against division by very small velocity with a check on line 546, but line 544 lacks similar protection for very small radius. If
myR_tmp1(4)is near zero (but positive), the relative errorerr(4)can become arbitrarily large or unstable.Apply this diff to use consistent safe-denominator logic for both terms:
- err(4) = abs((myR_tmp1(4) - myR_tmp2(4))/myR_tmp1(4)) - err(5) = abs((myV_tmp1(4) - myV_tmp2(4))/myV_tmp1(4)) - if (abs(myV_tmp1(4)) < verysmall) err(5) = 0._wp + err(4) = abs((myR_tmp1(4) - myR_tmp2(4))/max(abs(myR_tmp1(4)), verysmall)) + err(5) = abs((myV_tmp1(4) - myV_tmp2(4))/max(abs(myV_tmp1(4)), verysmall))Alternatively, zero out
err(4)whenmyR_tmp1(4)is very small, similar to line 546.Based on past review comments.
784-786: Consider using a named constant for velocity threshold.The hardcoded threshold
1.e-12_wpat line 784 for detecting non-oscillating bubbles is reasonable for double precision, but using a named constant (e.g.,velocity_thresholdor reusingverysmall/sgm_eps) would improve maintainability and make the intent clearer.Example:
if (max(abs(myV_tmp(1)), abs(myV_tmp(4))) < verysmall) then err_V = 0._wp end ifThis aligns with the check at line 546 and centralizes the threshold definition.
807-820: Clarify parameter name for vapor mass rate.The output parameter at line 813 is renamed from
fdMvdt_tmptoadvance_EL, but the latter is vague and doesn't convey that it represents the time derivative of vapor mass (dMv/dt). The original name was clearer.Consider reverting to a more descriptive name:
- real(wp), intent(out) :: advance_EL + real(wp), intent(out) :: fdMvdt_tmpOr use an even clearer name like
dMv_dtorvapor_mass_rate.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/simulation/m_bubbles.fpp(20 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{fpp,f90}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{fpp,f90}: Use 2-space indentation; continuation lines align beneath &
Use lower-case keywords and intrinsics (do, end subroutine, etc.)
Name modules with m_ pattern (e.g., m_transport)
Name public subroutines with s_ pattern (e.g., s_compute_flux)
Name public functions with f_ pattern
Keep subroutine size ≤ 500 lines, helper subroutines ≤ 150 lines, functions ≤ 100 lines, files ≤ 1000 lines
Limit routine arguments to ≤ 6; use derived-type params struct if more are needed
Forbid goto statements (except in legacy code), COMMON blocks, and save globals
Every argument must have explicit intent; use dimension/allocatable/pointer as appropriate
Call s_mpi_abort() for errors, never use stop or error stop
**/*.{fpp,f90}: Indent 2 spaces; continuation lines align under&
Use lower-case keywords and intrinsics (do,end subroutine, etc.)
Name modules withm_<feature>prefix (e.g.,m_transport)
Name public subroutines ass_<verb>_<noun>(e.g.,s_compute_flux) and functions asf_<verb>_<noun>
Keep private helpers in the module; avoid nested procedures
Enforce size limits: subroutine ≤ 500 lines, helper ≤ 150, function ≤ 100, module/file ≤ 1000
Limit subroutines to ≤ 6 arguments; otherwise pass a derived-type 'params' struct
Avoidgotostatements (except unavoidable legacy); avoid global state (COMMON,save)
Every variable must haveintent(in|out|inout)specification and appropriatedimension/allocatable/pointer
Uses_mpi_abort(<msg>)for error termination instead ofstop
Use!>style documentation for header comments; follow Doxygen Fortran format with!! @paramand!! @returntags
Useimplicit nonestatement in all modules
Useprivatedeclaration followed by explicitpublicexports in modules
Use derived types with pointers for encapsulation (e.g.,pointer, dimension(:,:,:) => null())
Usepureandelementalattributes for side-effect-free functions; combine them for array ...
Files:
src/simulation/m_bubbles.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_bubbles.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_bubbles.fpp
🧠 Learnings (10)
📚 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_bubbles.fpp
📚 Learning: 2025-11-24T21:50:16.713Z
Learnt from: CR
Repo: MFlowCode/MFC PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-24T21:50:16.713Z
Learning: Applies to src/simulation/**/*.{fpp,f90} : Wrap tight GPU loops with !$acc parallel loop gang vector default(present) reduction(...); add collapse(n) when safe; declare loop-local variables with private(...)
Applied to files:
src/simulation/m_bubbles.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_bubbles.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} : Mark GPU-callable helpers with `$:GPU_ROUTINE(function_name='...', parallelism='[seq]')` immediately after declaration
Applied to files:
src/simulation/m_bubbles.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} : Every argument must have explicit intent; use dimension/allocatable/pointer as appropriate
Applied to files:
src/simulation/m_bubbles.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} : Every variable must have `intent(in|out|inout)` specification and appropriate `dimension` / `allocatable` / `pointer`
Applied to files:
src/simulation/m_bubbles.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_bubbles.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_bubbles.fpp
📚 Learning: 2025-11-24T21:50:46.909Z
Learnt from: CR
Repo: MFlowCode/MFC PR: 0
File: .cursor/rules/mfc-agent-rules.mdc:0-0
Timestamp: 2025-11-24T21:50:46.909Z
Learning: Applies to **/*.{fpp,f90} : Use `wp` (working precision) parameter from `m_precision_select` instead of hardcoded precision like `real*8`
Applied to files:
src/simulation/m_bubbles.fpp
📚 Learning: 2025-11-24T21:50:16.713Z
Learnt from: CR
Repo: MFlowCode/MFC PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-24T21:50:16.713Z
Learning: Applies to src/simulation/**/*.{fpp,f90} : Avoid stop/error stop inside GPU device code
Applied to files:
src/simulation/m_bubbles.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). (16)
- GitHub Check: Github (ubuntu, mpi, debug, true)
- GitHub Check: Github (ubuntu, no-mpi, single, no-debug, false)
- GitHub Check: Github (ubuntu, mpi, debug, false)
- 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 (macos, mpi, no-debug, false)
- GitHub Check: Oak Ridge | Frontier (gpu-omp)
- GitHub Check: Georgia Tech | Phoenix (cpu)
- GitHub Check: Georgia Tech | Phoenix (gpu-omp)
- GitHub Check: Oak Ridge | Frontier (gpu-acc)
- GitHub Check: Georgia Tech | Phoenix (gpu-acc)
- GitHub Check: Oak Ridge | Frontier (cpu)
- GitHub Check: Coverage Test on CodeCov
- GitHub Check: Code Cleanliness Check
- GitHub Check: Build & Publish
🔇 Additional comments (4)
src/simulation/m_bubbles.fpp (4)
482-482: Good separation of initial and working step sizes.Introducing
h0to store the initial step size guess and usinghas the working variable improves code clarity and follows good adaptive time-stepping practice.Also applies to: 489-490
513-542: Efficient early-exit optimization for adaptive time-stepping.The early-exit checks at lines 513–516, 525–528, and 539–542 avoid unnecessary computation of subsequent Runge–Kutta stages when an earlier stage already exceeds the error tolerance. This is a standard and effective optimization for adaptive ODE solvers.
729-731: Robust sanity checks for negative bubble radius.The early-exit checks at lines 729–731, 746–748, and 763–765 detect physically invalid negative radii during Runge–Kutta stages and force step rejection by setting
err > adap_dt_tol. This improves the robustness of the adaptive time-stepper and prevents unphysical states from propagating.Also applies to: 746-748, 763-765
434-435: Verify whether wall temperatureTwscaling is missing.A previous reviewer flagged that the
grad_Tcalculation may be missing aTw(wall temperature) scaling factor that was present in the original implementation. The current dimensionless formula computes:grad_T = -Re_trans_T(iR0)*((fpb/pb0(iR0))*(fR/R0(iR0))**3 *(mass_g0(iR0) + mass_v0(iR0))/(mass_g0(iR0) + fmass_v) - 1._wp)Without access to the prior version or physics derivation, I cannot definitively confirm whether
Twshould multiply the entire expression to produce a dimensional temperature gradient.Run the following script to check the git history for this formula:
#!/bin/bash # Search git history for changes to grad_T calculation in m_bubbles git log -p --all -S "grad_T" -- "**/m_bubbles.f*" | head -200Based on past review comments.
|
@sbryngelson could you re-run the failed test suite? The error seems irrelevant to the code change. |
3 participants
User description
Description
This PR refactors the initialization process of sub-grid bubble models (EE and EL) as well as removing the assumption on normalization with
R0ref. It involves many things as listed below, but the overall goal is to reduce redundancy, inconsistency and assumptions.Key changes include:
EE and EL bubbles have independent initialization subroutines and especially EE bubbles with non-polytropic process have so many redundant initialization codes over multiple files. Now they are cleared and both EE and EL bubbles share common initialization subroutines in
common/m_helper.fpp.Some of bubble parameters are defined as
fluid_pp%and dimensional, while some others such as Ca, Web, and Re_inv are notfluid_ppand non-dimensional. Furthermore, EL bubbles require reference values as inputs (lag_params%) since non-dimensional quantities are computed inside the code. EE bubbles with non-polytropic process requiresprefandrhoref. All of these kind of random input parameters are re-organized asbub_pp%which are shared by EE and EL bubbles.bub_pp%parameters can be either dimensional or non-dimensional. These quantities should follow the normalization used for other input parameters, except for molecular weightsbub_pp%M_vandbub_pp%M_g. Thus, all the input parameters, except forbub_pp%M_vandbub_pp%M_g, should be consistent now.Minor changes include:
Related examples and tests case files are updated accordingly. In current master,
1D_exp_bubscreenarbitrarily setCa = 1, which is inconsistent with other parameters. This is corrected and thus the golden file is updated.Some of EE bubbles routines are refactored and a couple of checkers for non-physical behaviors are added
Subscript
_nindicating gas content inside bubbles is replaced with_g.Issue to be fixed in next PR:
_nand_v. Let me demonstrate this using one of example cases3D_lagrange_sphbubcollapse. This case's input includesAs usual, 1st fluid is the host liquid and 2nd fluid is the gas content in EL bubble. However,
s_start_lagrange_inputsreads the input parameters incorrectly as belowAnd in the other parts of the EL code,
_nis still treated as gas and_vas vapor. I think this is an obvious bug. But fixing this bug requires to update the golden file, which hinders validating changes introduced in this PR. I will fix this once this PR is merged.1D_qbmmand1D_poly_bubscreendo not have any acoustic source although they are in a bubble screen setup. When I add an acoustic source,1D_qbmmcrashes (even with the master branch).Fixes #(issue) [optional]
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?
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
Keller-Miksis + Non-polytropic
Keller-Miksis + Polytropic
Rayleigh-Plesset + Non-polytropic
Rayleigh-Plesset + Polytropic
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
Consolidated bubble model initialization by creating unified
s_initialize_bubbles_model()subroutine inm_helper.fpp, eliminating redundant initialization code across EE and EL bubble implementationsIntroduced new
subgrid_bubble_physical_parametersderived type (bub_pp) to centralize and organize all bubble-related parameters, replacing scattered definitions acrossfluid_pp%and other structuresReplaced
R0refwithEu(Euler number) as the primary nondimensional parameter for bubble pressure calculationsStandardized nomenclature throughout codebase by replacing subscript
_nwith_gfor gas-related variables (e.g.,R_n→R_g,mass_n0→mass_g0,gamma_n→gam_g)Updated all example cases and test configurations to use the new
bub_pp%structure with consistent parameterizationRefactored adaptive time-stepping logic in bubble dynamics with improved error checking and validation
Added validation checks for non-physical behaviors including negative bubble radius detection
Updated MPI communication across preprocessing, simulation, and post-processing modules to broadcast
bub_pp%parametersSimplified Lagrange bubble initialization by removing
s_start_lagrange_inputs()subroutineUpdated QBMM module with normalized gas nomenclature and improved numerical stability checks
Diagram Walkthrough
File Walkthrough
13 files
m_bubbles.fpp
Normalize bubble nomenclature and refactor adaptive time-steppingsrc/simulation/m_bubbles.fpp
prefwithEu(Euler number) in bubble pressure calculations_nto_gthroughout for gas-related variables (e.g.,R_n→R_g,mass_n0→mass_g0)gamma_n,phi_vn,phi_nvtogam_g,phi_vg,phi_gvand tolerance comparisons
handling
m_qbmm.fpp
Update QBMM module with normalized gas nomenclaturesrc/simulation/m_qbmm.fpp
mass_n0tomass_g0and_nsubscriptsto
_gverysmallwithsgm_epsfor numerical stability checksT_barvariablenew nomenclature
m_assign_variables.fpp
Update variable assignment with bubble parameters structuresrc/pre_process/m_assign_variables.fpp
fluid_pp(1)%pvreferences tobub_pp%pvR0ref,p0ref,rho0refcalculations
m_bubbles_EE.fpp
Refactor Euler-Euler bubble source computation with validationsrc/simulation/m_bubbles_EE.fpp
logic
code organization
small_alfthreshold check for void fraction cutoffm_variables_conversion.fpp
Update variable conversion with normalized gas nomenclaturesrc/common/m_variables_conversion.fpp
mass_g0instead ofmass_n0conversions
case.py
Refactor bubble parameters to use bub_pp structureexamples/2D_bubbly_steady_shock/case.py
conventions
n_tait,B_tait,gamma_n,M_n, etc.) withstandardized names (
gam_l,pi_inf_l,gam_g,M_g, etc.)pref,rhoref,Ca,Web,Re_invparameters and replaced with newbub_pp%structure for bubble parametersscheme
case.py
Refactor QBMM bubble case with new bub_pp parametersexamples/1D_qbmm/case.py
naming
Ca,Web,Re_inv,pref,rhoref)bub_pp%parameters including thermal properties(
k_v,k_g,cp_v,cp_g,R_v,R_g)case.py
Refactor bubble screen case with bub_pp parametersexamples/1D_bubblescreen/case.py
naming conventions
pref,rhoref,Ca,Web,Re_invparametersbub_pp%structure for bubble parameters with propernormalization
wavelength
case.py
Refactor polydisperse bubble screen caseexamples/1D_poly_bubscreen/case.py
Ca,Web,Re_inv,pref,rhoref)bub_pp%structure for bubble parameters with consistentnormalization
case.py
Refactor adaptive bubble collapse caseexamples/0D_bubblecollapse_adap/case.py
naming
Ca,Web,Re_inv,pref,rhoref)bub_pp%structure for bubble parameters with thermal propertiescase.py
Refactor whale bubble annulus caseexamples/2D_whale_bubble_annulus/case.py
Ca,Web,Re_inv,R0ref)bub_pp%structure for bubble parametershandling
case.py
Refactor Lagrangian bubble collapse caseexamples/3D_lagrange_shbubcollapse/case.py
gamma_g→gam_g,gamma_v→gam_v,mu_gas→mu_g)lag_params%reference parameters (c0,rho0,T0,x0,Thost)bub_pp%structure for Lagrangian bubble parameterswith thermal properties
fluid_pp()%definitionscase.py
Refactor Lagrangian bubble screen caseexamples/3D_lagrange_bubblescreen/case.py
gamma_g→gam_g,gamma_v→gam_v)lag_params%reference parametersbub_pp%structure for Lagrangian bubble parametersfluid_pp()%definitions12 files
m_helper.fpp
Consolidate bubble model initialization into unified subroutinesrc/common/m_helper.fpp
s_initialize_bubbles_model()subroutine consolidatingbubble initialization logic
s_initialize_nonpoly()to usebub_pp%parameters instead offluid_pp%bub_pp%structure_n,_v) with new nomenclature (_g,_v)m_global_parameters.fpp
Introduce subgrid bubble parameters structure and reorganize globalssrc/simulation/m_global_parameters.fpp
bub_ppstructure of typesubgrid_bubble_physical_parametersR0refwithEuas primary nondimensional parameter_gfor gas,
_vfor vapor)fluid_pp%structurem_global_parameters.fpp
Add bubble parameters structure to preprocessing modulesrc/pre_process/m_global_parameters.fpp
bub_ppstructure declaration for subgrid bubble parametersconventions
fluid_pp%initializationbub_pp%structurem_bubbles_EL.fpp
Simplify Lagrange bubble initialization and normalize nomenclaturesrc/simulation/m_bubbles_EL.fpp
s_start_lagrange_inputs()subroutine (initialization nowhandled by
s_initialize_bubbles_model())R_ntoR_gandk_nltok_glcp_ntocp_gfor gas-phase specific heatfluid_pp(num_fluids)%D_vtovdm_global_parameters.fpp
Add bubble parameters structure to post-processing modulesrc/post_process/m_global_parameters.fpp
bub_ppstructure for subgrid bubble parameters_gfor gas,_vfor vapor)
fluid_pp%structurem_start_up.fpp
Consolidate bubble initialization in startup modulesrc/simulation/m_start_up.fpp
s_initialize_bubbles_model()callbub_ppparameter to subroutine interfacecode
m_derived_types.fpp
Create dedicated subgrid bubble parameters derived typesrc/common/m_derived_types.fpp
physical_parameterstypesubgrid_bubble_physical_parametersderived type withcomprehensive bubble properties
properties, thermodynamic constants)
m_mpi_proxy.fpp
Update MPI communication for bubble parameters structuresrc/pre_process/m_mpi_proxy.fpp
fluid_pp%loopbub_pp%structure parametersm_start_up.fpp
Consolidate bubble initialization in preprocessing startupsrc/pre_process/m_start_up.fpp
bub_ppparameter to input reading interfaces_initialize_bubbles_model()m_mpi_proxy.fpp
Update MPI communication for bubble parameters structuresrc/simulation/m_mpi_proxy.fpp
fluid_pp%MPI broadcast loopbub_pp%structurem_mpi_proxy.fpp
Update post-processing MPI communication for bubble parameterssrc/post_process/m_mpi_proxy.fpp
D_vfromfluid_pp%MPI broadcastbub_pp%structure parametersm_start_up.fpp
Consolidate bubble initialization in post-processing startupsrc/post_process/m_start_up.fpp
bub_ppparameter to input reading interfaces_initialize_bubbles_model()2 files
m_riemann_solvers.fpp
Add NaN detection debugging in Riemann solversrc/simulation/m_riemann_solvers.fpp
calculations
case_validator.py
Update bubble validation constraintstoolchain/mfc/case_validator.py
R0refrequirement in non-polytropicbubble mode
polytropic =Fandthermal = 33 files
m_time_steppers.fpp
Add placeholder variables for time-stepping diagnosticssrc/simulation/m_time_steppers.fpp
denomandnbubble(currently unused)m_rhs.fpp
Add placeholder variables for RHS computationsrc/simulation/m_rhs.fpp
denomandnbubble(currently unused)golden-metadata.txt
Update test golden file metadatatests/5CAA4E68/golden-metadata.txt
2 files
case.py
Refactor bubble screen example with consistent parameterizationexamples/1D_exp_bubscreen/case.py
definitions
bub_pp%structureCa = 1with physically consistent cavitation numbercalculation
case_dicts.py
Update parameter dictionaries with bub_pp structuretoolchain/mfc/run/case_dicts.py
Ca,Web,Re_invfrom parameter type definitionsbub_pp%parameter definitions for both PRE_PROCESSand SIMULATION dictionaries
fluid_pp()%parameter list(moved to
bub_pp%)bub_pp%parameters to POST_PROCESS dictionary2 files
cases.py
Update test cases with new bubble parameter structuretoolchain/mfc/test/cases.py
fluid_pp(2)%bubble gas parameters from test casedefinitions
bub_pp%parameters for bubble dynamics testsalter_bubblesandalter_viscositytest functions with newparameter structure
bub_pp%parameters to Lagrangian bubble test case with thermalproperties
case.py
Update test case parameterstoolchain/mfc/test/case.py
Ca,Web,Re_invparameters from test case definitionR0refparameter from test caseprefandrhorefparameters (kept but not shown in diffcontext)
1 files
Summary by CodeRabbit
New Features
Configuration Updates
Removals
✏️ Tip: You can customize this high-level summary in your review settings.