Fix missing qv term in the Lagrange bubble initial pressure - #1709
Fix missing qv term in the Lagrange bubble initial pressure#1709sbryngelson wants to merge 2 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Fixes incorrect initial Lagrangian bubble pressure when a nonzero heat-of-formation term (qv) is configured, and adds a targeted regression test/golden to prevent recurrence.
Changes:
- Include the missing
qvterm in the stiffened-gas pressure inversion used to seed Lagrangian bubble pressure. - Add a new lag-bubble test configuration with
fluid_pp(1)%qv = 0.01to exercise the previously untested path. - Add a new golden (
43EA05B4) to validate the behavior.
Reviewed changes
Copilot reviewed 3 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
src/simulation/m_bubbles_EL.fpp |
Fixes pressure inversion by subtracting qv in the bubble initialization path. |
toolchain/mfc/test/cases.py |
Adds a constrained regression case that sets qv nonzero for lag bubbles. |
tests/43EA05B4/golden-metadata.txt |
Adds metadata for the new golden tied to the new regression case. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
Claude Code Review Head SHA: b3431b4 Files changed:
Findings:
|
s_add_bubbles open-coded the stiffened-gas pressure inversion and omitted the
qv (heat of formation) term:
pliq = (E - dynP - pi_inf)/gamma
The canonical inversion in s_compute_pressure (m_variables_conversion.fpp:71)
is (energy - dyn_p - pi_inf - qv)/gamma. The omission is clearly accidental:
qv is an output argument of the s_convert_to_mixture_variables call one line
above and is never read anywhere else in that scope.
pliq seeds gas_p(bub_id, 1), so every Lagrangian bubble started from a wrong
internal pressure. This is physics, not diagnostics. It also propagates to the
initial gas mass and the bubble natural frequency, which are both derived from
gas_p in the same routine.
Consistency with the running solver settles which quantity pliq should be:
s_get_pinf, the Maeda and Colonius (2018) subgrid closure that supplies the
bubble driving pressure at every later step, interpolates q_prim_vf(eqn_idx%E),
and that field is filled by s_compute_pressure with qv included. Initializing
from an inversion that omits qv left each bubble out of equilibrium with its
own driving pressure by qv/gamma.
Only s_add_bubbles needs the change; the restart path reads gas_p straight from
the restart file rather than recomputing it.
Dormant when qv = 0, the default, and every existing lag-bubble test leaves it
there, which is why this survived.
Adds a 2D one-way-coupled case with qv /= 0. The golden lands in a follow-up
commit so that it is generated from a clean tree.
Fixes #1706
Generated from a clean checkout of the preceding commit so the recorded provenance corresponds exactly to committed sources. The golden captures beta, the Lagrangian void fraction, alongside the conservative variables. beta follows the bubble radius, which is driven by the initial gas pressure, so the defect is observable even under one-way coupling where the bubbles do not feed back into the Eulerian field.
b3431b4 to
9191c9a
Compare
|
Both Copilot points were valid and are addressed in the force-push ( Label Dirty working tree. Also correct, and the recorded SHA which is the parent commit in this PR. One thing the restructure caught. After committing the fix, my original verification method silently stopped working — Worth stating explicitly because a regression test that cannot fail is worse than no test, and the false pass looked identical to a real one. Also updated the description with two things I had missed on the first pass: the fix can newly trigger the vapour-pressure and negative-gas-mass aborts for |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #1709 +/- ##
=======================================
Coverage 60.77% 60.77%
=======================================
Files 83 83
Lines 20872 20872
Branches 3101 3101
=======================================
Hits 12685 12685
Misses 6121 6121
Partials 2066 2066 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Fixes #1706.
The defect
src/simulation/m_bubbles_EL.fppopen-codes the stiffened-gas pressure inversion and drops theqv(heat of formation) term:The canonical inversion,
s_compute_pressureinm_variables_conversion.fpp:71, isThe omission looks accidental rather than a deliberately different quantity:
qvis an output argument of thes_convert_to_mixture_variablescall on the line above, and it is never read anywhere else in that scope. It is computed and discarded.Impact
pliqis too large byqv/gamma, and it seeds the initial bubble gas pressure:So every Lagrangian bubble starts from the wrong internal pressure. This is physics, not diagnostic output.
Dormant when
qv = 0, which is the default. It triggers for any case settingfluid_pp(i)%qv— the phase-change and reactive-burn configurations.Why it survived
No test exercised the combination. Exactly one golden case sets
bubbles_lagrange, and it leavesqvat zero; the cases that setqvare phase-change and reactive-burn, none of which use Lagrangian bubbles. The suite could not have caught this.Test
Adds a 2D one-way-coupled Lagrange bubble case with
fluid_pp(1)%qv = 0.01, restricted to a single configuration so it contributes one golden (F428FDC0).The golden captures
beta, the Lagrangian void fraction, alongside the conservative variables.betafollows the bubble radius, which is driven by the initial gas pressure, so the error is observable even under one-way coupling.Verification
Built with GNU 15.2.0, MPI, on macOS. The negative result is the one that matters — without it, a green suite would prove nothing here, since the pre-existing cases are all
qv = 0.No existing golden moved: the change is confined to a path only reachable with
bubbles_lagrange, and no existing lag case has nonzeroqv.Behaviour change worth flagging
The correction shifts
pliqby-qv/gamma, andgamma > 0always, so the direction follows the sign ofqv. Forqv > 0the initial bubble pressure drops, which brings two existing guards in the same routine within reach:A case with
qv > 0and Lagrangian bubbles that previously ran can now abort at startup. That is the correct outcome — it was running on an inflated bubble pressure that masked a setup the model rejects — but it converts a silently wrong answer into a hard stop, so it is a user-visible change rather than a pure numerical shift. Forqv < 0(as in the phase-change examples, which useqv = -1.167e6for fluid 1) the pressure rises and the guards become less reachable.gas_palso feeds the initial gas mass and the bubble natural frequency in the same routine, so both are corrected by the same change.Adjacent issue found while reviewing
s_add_bubblesalso assigns the module-level globalpreffrom the last-added bubble'sgas_p(line 383).prefis a documented user-settable case parameter that the four-equation EOS branch reads. This PR changes the value it gets overwritten with, but the overwrite itself is pre-existing and looks wrong independently. Filed as #1710 rather than addressed here.Context
First of three PRs against #1708. The root cause is that the EOS algebra has several independent definitions, and
s_compute_pressureis awkward enough to call — it demands anum_speciesarray and anintent(inout)temperature — that call sites open-code the expression instead. This PR fixes only the bug; deduplication follows separately so that any behavior change lands in a PR whose purpose is to change behavior.