Complete form of the SG EoS #1057
Conversation
This PR is to expand the SG EoS to its general form. Such a change is needed for the PC model to work on both 5 and 6 equation models
|
CodeAnt AI is reviewing your PR. |
|
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. WalkthroughThis PR introduces a new internal-energy reference variable Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant Enthalpy as s_compute_enthalpy
participant SpeedSound as s_compute_speed_of_sound
Caller->>Enthalpy: compute H (pres,rho,gamma,pi_inf,vel,...)
Note right of Enthalpy `#DDEEFF`: returns H and qv (qv now an output)
Enthalpy-->>Caller: H, qv
Caller->>SpeedSound: compute c (pres,rho,gamma,pi_inf,H,adv,vel_sum, ..., qv)
Note right of SpeedSound `#F6E7D7`: non-bubble path uses qv: c = (H - 0.5*vel_sum - qv/rho)/gamma
SpeedSound-->>Caller: c
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
PR Reviewer Guide 🔍Here are some key observations to aid the review process:
|
codeant-ai
bot
added
the
size:M
![qodo-merge-pro[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
|
CodeAnt AI finished reviewing your PR. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/simulation/m_riemann_solvers.fpp (1)
361-375: Makeqv_avgprivate inside every GPU loop
qv_avgis newly introduced for the averaged stiffened-gas offset but it isn’t added to theprivateset of the GPU loops (e.g., this@:GPU_PARALLEL_LOOPand the analogous HLLC loop around Line 2003). On accelerators that share loop variables by default, this leavesqv_avgas a race-prone shared scalar, so different cell evaluations stomp on each other’s value and the averaged speed of sound becomes numerically garbage. Please addqv_avgto the private lists for all of these kernels.- $:GPU_PARALLEL_LOOP(... private='[..., qv_L, qv_R, c_L, c_R, G_L, G_R, rho_avg, H_avg, c_avg, gamma_avg, ...]') + $:GPU_PARALLEL_LOOP(... private='[..., qv_L, qv_R, qv_avg, c_L, c_R, G_L, G_R, rho_avg, H_avg, c_avg, gamma_avg, ...]')…and repeat for every other
@:GPU_PARALLEL_LOOPthat usesqv_avg.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
src/common/m_variables_conversion.fpp(3 hunks)src/post_process/m_data_output.fpp(3 hunks)src/post_process/m_start_up.fpp(1 hunks)src/simulation/m_cbc.fpp(1 hunks)src/simulation/m_data_output.fpp(4 hunks)src/simulation/m_riemann_solvers.fpp(9 hunks)src/simulation/m_sim_helpers.fpp(1 hunks)src/simulation/m_time_steppers.fpp(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (13)
- GitHub Check: Spell Check
- GitHub Check: Detect File Changes
- GitHub Check: Detect File Changes
- GitHub Check: Detect File Changes
- GitHub Check: Build & Publish
- GitHub Check: PMD
- GitHub Check: Detect File Changes
- GitHub Check: Detect File Changes
- GitHub Check: Lint Source
- GitHub Check: Detect File Changes
- GitHub Check: Formatting
- GitHub Check: Lint Toolchain
- GitHub Check: cubic · AI code reviewer
🔇 Additional comments (19)
src/simulation/m_cbc.fpp (1)
857-857: LGTM! Signature update is consistent.The call to
s_compute_speed_of_soundcorrectly includesqvas the new final parameter, andqvis properly obtained from the mixture variable conversion at lines 817-819.src/post_process/m_start_up.fpp (1)
703-703: LGTM! Post-processing signature updated correctly.The call to
s_compute_speed_of_soundnow includesqv_sf(i, j, k)as the final parameter, consistent with the PR's goal of incorporating the thermal SG EoS parameter.src/simulation/m_time_steppers.fpp (1)
712-740: LGTM! Proper threading of qv through time-stepping.The changes correctly:
- Declare
qvas a local variable (line 712)- Include it in the GPU private list (line 729)
- Obtain it as an output from
s_compute_enthalpy(lines 734, 736)- Pass it to
s_compute_speed_of_sound(line 740)The implementation maintains the correct data flow for the new thermal SG EoS parameter.
src/simulation/m_data_output.fpp (2)
276-289: LGTM! Consistent threading through run-time information.The changes in
s_write_run_time_informationproperly:
- Declare
qvas a local variable- Add it to the GPU private list
- Obtain it from
s_compute_enthalpy- Pass it to
s_compute_speed_of_sound
1296-1296: Code verified as correct—no changes needed.The inline enthalpy formula
((gamma + 1._wp)*pres + pi_inf)/rhois correct for the stiffened gas EoS. Thes_compute_speed_of_soundfunction properly accounts forqvby subtractingqv/rhofrom the enthalpy (line 1656 ofsrc/common/m_variables_conversion.fpp). This design—computing/passing enthalpy withoutqvand handling the reference energy separately inside the function—is consistent throughout the codebase (e.g.,m_cbc.fppline 854,m_riemann_solvers.fppmultiple locations). The physics is accurate.src/post_process/m_data_output.fpp (4)
1585-1585: Variable declaration looks correct.The addition of
qvto the variable declaration list is appropriate and follows the existing code style.
1629-1629: Accumulation formula is correct (assuming the initialization bug is fixed).The formula
qv = qv + adv(l)*q_prim_vf(l)%sf(i,j,k)*fluid_pp(l)%qvcorrectly computes the volume-fraction-weighted internal energy reference. The units are dimensionally consistent: volume fraction × partial density × specific energy reference = energy per volume (pressure units), which matches the usage in the enthalpy formula at line 1632.
1632-1632: Enthalpy formula correctly implements the general SG EoS.The addition of
qvto the enthalpy calculation implements the thermal SG EoS parameter as described in the PR objectives. The formulaH = ((gamma + 1._wp)*pres + pi_inf + qv)/rhois dimensionally consistent and correctly incorporates the internal energy reference term.
1634-1636: Speed of sound call correctly includesqvparameter.The addition of
qvas the final parameter tos_compute_speed_of_soundis consistent with the PR objective to compute speed of sound correctly for the 5-equation model. The parameter is passed after computing bothH(which includesqv) and the accumulatedqvvalue itself.src/common/m_variables_conversion.fpp (10)
426-433: LGTM: Density-weighted qv accumulation is physically sound.The density-weighted accumulation
qv = qv + alpha_rho_K(i)*qvs(i)correctly computes the mixture's energy reference by summing partial density contributions from each fluid. This pattern is consistently applied across the codebase (lines 300, 313, 514, 573, 580).
143-149: LGTM: qv correctly integrated into pressure calculations.The qv term is consistently subtracted from the energy before computing pressure across all branches (MHD, standard, bubbles_euler). This correctly implements the Stiffened Gas EoS form:
p = (gamma - 1) * rho * (e - q) - gamma * pi_inf, which matches the PR objectives.
626-650: LGTM: Proper initialization of qvs and qvps arrays.The qv-related arrays are correctly allocated and initialized from the
fluid_ppstructure for all fluids, with proper GPU memory management. The pattern follows the existing initialization ofgammas,pi_infs, etc.
1586-1595: LGTM: Complete deallocation of qv-related arrays.All qv-related memory (
qv_sf,qvs,qvps) is properly deallocated, preventing memory leaks.
826-826: LGTM: qv properly threaded through conservative-to-primitive conversion.The
qv_Kvariable is correctly declared, computed from mixture variables, and passed tos_compute_pressure, ensuring the qv term participates in the pressure calculation as intended.Also applies to: 884-891, 1038-1041
1303-1310: LGTM: qv correctly added to energy in primitive-to-conservative conversion.The qv term is properly added to the conservative energy variable (lines 1305, 1310), which is the inverse operation of subtracting it during pressure computation. This maintains consistency with the Stiffened Gas EoS.
1524-1526: LGTM: qv included in flux energy computation.The qv_K term is correctly included in the energy calculation for flux variables, maintaining consistency with the conservative energy formulation.
1651-1651: Manually verify H computation and qv consistency in speed of sound formula.The verification encountered limitations due to code generation and preprocessing in the file. The subroutine
s_compute_speed_of_soundat line 1604 takesHandqvas separate input parameters, and line 1651 subtracts both(H - 5.e-1*vel_sum - qv/rho)/gamma.However:
- No call sites to
s_compute_speed_of_soundwere found in the codebase- No references to
s_convert_primitive_to_flux_variableswere found- The file uses preprocessor directives (
#ifndef MFC_PRE_PROCESS) suggesting build-time code generationPlease manually verify:
- Where
His computed before being passed tos_compute_speed_of_sound- Whether
Halready includes theqvterm in its upstream calculation- Whether the subtraction of
qv/rhoon line 1651 is correct (not a double-subtraction)- Build/generation scripts to understand how this subroutine is actually called
221-221: Hardcodedqv = 0._wpat line 221 is flagged as incomplete work and needs clarification.Line 221 explicitly includes a TODO-like comment ("keep this value nill for now. For future adjustment"), indicating this is known incomplete implementation for the gamma/pi_inf model (model_eqns==1). However,
qvis used downstream in pressure, energy, and speed of sound calculations, so hardcoding it to zero may produce incorrect results.The suggested fix in the original comment (using
fluid_pp(1)%qv) references a different function's pattern and may not be the intended approach here. Clarify:
- Should model_eqns==1 extract
qvfrom a field variable (would require addingqv_idx, similar togamma_idx/pi_inf_idx)?- Or should it use
fluid_pp(1)%qvdespite the current pattern of extracting from fields?- If
qv=0is intentional for this model, the downstream calculations usingqvneed review.
1636-1638: The code is correct—model_eqns == 3 is the 6-equation model, not 5-equation, and uses a different speed of sound formulation.The verification confirms that line 1637 is correct as-is. The speed of sound formula
c = sum(adv*gs_min*(pres + ps_inf))/rhois the appropriate formulation for the 6-equation model (Saurel et al., 2009), which uses mixture-based computation via volume fractions and stiffness terms. Theqvparameter is only utilized in the else branch (model_eqns == 1, the Gamma/Pi_inf model) at line 1651, which is the intended design: different equation systems use different physical formulations. Theqvparameter is necessary in the function signature because all call sites provide it for model_eqns == 1 cases. No changes required.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #1057 +/- ##
==========================================
+ Coverage 44.34% 44.35% +0.01%
==========================================
Files 71 71
Lines 20578 20589 +11
Branches 1990 1990
==========================================
+ Hits 9125 9133 +8
- Misses 10308 10311 +3
Partials 1145 1145 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
CodeAnt AI is running Incremental review |
codeant-ai
bot
added
size:M
|
CodeAnt AI Incremental review completed. |
|
@JRChreim Any idea why the tests are failing with minor tolerance violations? As far as I can tell from the changes, the answers shouldn't change as long as qv is zero. |
|
CodeAnt AI is running Incremental review |
|
Never mind. I think I fixed it. |
codeant-ai
bot
added
size:M
![codeant-ai[bot]](https://avatars.githubusercontent.com/in/646884?s=60&v=4){kind=small}
|
CodeAnt AI Incremental review completed. |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 2 files (reviewed changes from recent commits).
Prompt for AI agents (all 1 issues)
Understand the root cause of the following 1 issues and fix them.
<file name="src/simulation/include/inline_riemann.fpp">
<violation number="1" location="src/simulation/include/inline_riemann.fpp:36">
Roe-averaged `qv` is computed with enthalpy values and `sqrt(qv)` weights, so the reference energy passed to the SG EOS is wrong; weight it with `sqrt(rho)` and average the actual `qv` values.</violation>
</file>
Reply to cubic to teach it or ask questions. Re-run a review with @cubic-dev-ai review this PR
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/simulation/m_riemann_solvers.fpp (1)
1938-1943: Ensureflux_ene_eis private in the 5‑equation HLLC GPU loop when elasticity is enabledIn the 5‑equation HLLC branch’s
GPU_PARALLEL_LOOP(starting around Line 3095), theprivatelist does not includeflux_ene_e, yet:
flux_ene_eis a subroutine-local scalar (declared near Line 1970).- It is used and reassigned inside the loop when
elasticityis true:if (elasticity) then flux_ene_e = 0._wp ... flux_ene_e = flux_ene_e - ... ... flux_rs${XYZ}$_vf(j, k, l, E_idx) = flux_rs${XYZ}$_vf(j, k, l, E_idx) + flux_ene_e end ifIf elasticity can ever be active for this branch, sharing
flux_ene_eacross iterations/threads on the device would introduce a data race and corrupt the energy flux.I suggest adding
flux_ene_eto theprivateclause of this loop for safety:- $:GPU_PARALLEL_LOOP(collapse=3, private='[Re_max, i, q, T_L, T_R, idx1, idxi, vel_L_rms, ... - Yi_avg,Phi_avg, h_iL, h_iR, h_avg_2, G_L, G_R]', copyin='[is1, is2, is3]') + $:GPU_PARALLEL_LOOP(collapse=3, private='[Re_max, i, q, T_L, T_R, idx1, idxi, vel_L_rms, ... + Yi_avg,Phi_avg, h_iL, h_iR, h_avg_2, flux_ene_e, G_L, G_R]', copyin='[is1, is2, is3]')If by construction
elasticityis guaranteed to be false formodel_eqnscorresponding to this branch, it would still be clearer and more robust to treatflux_ene_eas private here.Also applies to: 3095-3310, 3435-3451
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/simulation/m_riemann_solvers.fpp(14 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (17)
- GitHub Check: Code Cleanliness Check
- GitHub Check: Github (macos, mpi, debug, false)
- GitHub Check: Github (ubuntu, no-mpi, single, no-debug, false)
- GitHub Check: Github (macos, mpi, no-debug, false)
- GitHub Check: Github (ubuntu, mpi, no-debug, false)
- GitHub Check: Self Hosted (cpu, none, frontier)
- GitHub Check: Github (ubuntu, mpi, no-debug, true)
- GitHub Check: Github (ubuntu, mpi, debug, true)
- GitHub Check: Github (ubuntu, mpi, debug, false)
- GitHub Check: Self Hosted (cpu, none, gt)
- GitHub Check: Self Hosted (gpu, acc, gt)
- GitHub Check: Self Hosted (gpu, omp, gt)
- GitHub Check: Self Hosted (gpu, omp, frontier)
- GitHub Check: Self Hosted (gpu, acc, frontier)
- GitHub Check: Coverage Test on CodeCov
- GitHub Check: cubic · AI code reviewer
- GitHub Check: Build & Publish
🔇 Additional comments (5)
src/simulation/m_riemann_solvers.fpp (5)
320-323: HLL solver: qv handling and speed-of-sound calls look consistent
qv_L/qv_Rare accumulated usingalpha_rho_* * qvs(i)and included in energyE_*in the same way asgamma_*andpi_inf_*, which keeps the SG energetics consistent.qv_avgis declared and treated as a private scalar in the GPU loop, and then passed intos_compute_speed_of_soundalong with the updated signature(…, vel_*_rms, ..., c_*, qv_*).This block is internally consistent with the SG+qv formulation and with how the other Riemann solvers are updated.
Also applies to: 361-452, 632-643
1129-1219: LF solver: qv propagation into sound-speed is correct
qv_L/qv_Rare initialized, built fromalpha_rho_* * qvs(i), and then added toE_*in non‑chemistry branches in the same way as in HLL/HLLC.- The GPU loop’s
privatelist now includesqv_Landqv_R, avoiding shared-state issues on accelerators.- Both left and right calls to
s_compute_speed_of_sounduse the new trailingqvargument in the expected position:(..., vel_*_rms, 0._wp, c_*, qv_*).Looks correct and symmetric between left and right states.
Also applies to: 1359-1364
1938-1943: HLLC 6‑equation (model_eqns == 3): qv integration is coherent
qv_L/qv_Rare reset to zero, accumulated withqL/R_prim * qvs(i), and included inE_L/E_Rprior to computingH_L/H_R, keeping energy consistent with the extended SG EOS.qv_avgis declared, made private in the GPU loop, and passed into the average-states_compute_speed_of_soundcall alongsiderho_avg,H_avg, andgamma_avg.- Left/right and average sound-speed calls all pass
qv_*as the final argument, preserving the previous argument ordering.This branch’s treatment of qv matches the intended SG generalization.
Also applies to: 2000-2013, 2028-2078, 2098-2176
1938-1943: HLLC 4‑equation (model_eqns == 4): qv usage is consistent with other branches
qv_L/qv_Rare built fromalpha_rho_* * qvs(i)and included inE_L/E_Rand thus inH_L/H_R, same pattern as in the 6‑equation branch.- The GPU
privatelist is updated to includeqv_L,qv_R, andqv_avg.- All three
s_compute_speed_of_soundcalls use the new trailingqvparameter correctly.No correctness or threading red flags here.
Also applies to: 2414-2423, 2452-2487
3650-3753: HLLD solver: qv threading into the MHD SG EOS is clean
- The new
riemann_states :: qvmember is accumulated alongsiderho,gamma, andpi_inffromalpha_rho_*andalpha_*.E%L/E%RandH_no_mag%L/H_no_mag%Rincludeqv%L/qv%Rconsistently with the extended SG equation of state.- Left and right calls to
s_compute_speed_of_sounduseqv%Landqv%Ras the final argument, then feedcinto the fast magnetosonic speed calculation.No issues spotted in this integration; the HLLD path stays self-consistent.
This comment was marked as duplicate.
This comment was marked as duplicate.
Sorry, something went wrong.
Thank you. I missed that |
Including a suggestion from coderabbitai, effectively initializing qv_avg to input parameter on 's_compute_speed_of_sound'
|
CodeAnt AI is running Incremental review |
codeant-ai
bot
added
size:M
|
CodeAnt AI Incremental review completed. |
|
CodeAnt AI is running Incremental review |
codeant-ai
bot
added
size:M
|
CodeAnt AI Incremental review completed. |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/simulation/m_riemann_solvers.fpp (1)
2757-2855: Fix from past review: qv_avg now initialized in bubbles-euler avg_state==2This addresses the prior critical issue:
- When
avg_state == 2, you now compute
rho_avg,H_avg,gamma_avgas simple arithmetic means, andqv_avg = 0.5*(qv_L + qv_R)(Line 2853).This ensures
qv_avgis always defined before the subsequent call tos_compute_speed_of_soundthat uses it, eliminating the uninitialized‑variable bug in this region (and matching the pattern used for other averages).
🧹 Nitpick comments (2)
src/simulation/m_riemann_solvers.fpp (2)
318-323: Add qv_avg with consistent initialization in HLL solver
qv_avgis declared and added to the GPUprivatelist, and is passed tos_compute_speed_of_soundforc_avg. This is correct assuming@:compute_average_state()fillsqv_avgfor the models using an average state.Please double‑check that
@:compute_average_state()indeed assignsqv_avgfor every configuration whereavg_stateleads to the average-speed computation; otherwise, add an explicitqv_avginitialization alongsiderho_avg,H_avg, andgamma_avg.
2665-2721: bubbles_euler ME2: qv_L/qv_R initialized robustly across num_fluids casesIn this branch:
qv_L/qv_Rare zeroed and then consistently filled either in the full loop (num_fluids > 2with or withoutmpp_lim) or via direct single‑fluid assignments (num_fluids == 1).- This covers all
num_fluidsconditions used in this block;qv_*cannot remain uninitialized on any path.The remaining E/H formulas (currently omitting
+ qvfromE_*) are unchanged from prior behavior except for qv usage in the EOS viac_*; this looks intentional, but you may want to confirm consistency with the SG definition for this reduced model.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/simulation/m_riemann_solvers.fpp(15 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (17)
- GitHub Check: Self Hosted (cpu, none, gt)
- GitHub Check: Self Hosted (gpu, omp, frontier)
- GitHub Check: Self Hosted (cpu, none, frontier)
- GitHub Check: Github (ubuntu, no-mpi, single, no-debug, false)
- GitHub Check: Self Hosted (gpu, omp, gt)
- GitHub Check: Self Hosted (gpu, acc, gt)
- GitHub Check: Self Hosted (gpu, acc, frontier)
- GitHub Check: Github (ubuntu, mpi, no-debug, false)
- GitHub Check: Github (macos, mpi, no-debug, false)
- GitHub Check: Github (macos, mpi, debug, false)
- GitHub Check: Github (ubuntu, mpi, no-debug, true)
- GitHub Check: Github (ubuntu, mpi, debug, false)
- GitHub Check: Github (ubuntu, mpi, debug, true)
- GitHub Check: Coverage Test on CodeCov
- GitHub Check: Code Cleanliness Check
- GitHub Check: cubic · AI code reviewer
- GitHub Check: Build & Publish
🔇 Additional comments (17)
src/simulation/m_riemann_solvers.fpp (17)
361-363: GPU private list correctly extended to include qv variablesThe HLL solver’s GPU loop
privatelist now includesqv_L,qv_R, andqv_avg, matching their use inside the loop and preventing race conditions on GPUs. No further changes needed here.
632-643: Speed-of-sound calls in HLL solver now consistently pass qvAll three calls to
s_compute_speed_of_soundin HLL (left, right, and average states) now pass the appropriateqv_*value. This keeps the SG EOS consistent for both one‑sided and averaged states, providedqv_avgis initialized as noted earlier.
1128-1130: LF solver GPU private list includes new qv stateIn the Lax–Friedrichs solver,
qv_Landqv_Rare added to the GPUprivatelist and are used later in the sound‑speed and energy expressions. This matches their use; noqv_avgis needed since only side speeds are computed here. Looks good.
1359-1364: LF solver speed-of-sound calls correctly extended with qvThe LF solver now passes
qv_Landqv_Rintos_compute_speed_of_sound. This is consistent with how E and H are constructed and should preserve behavior whenqvsare zero while enabling non‑zero qv support.
1940-1942: qv_avg added to HLLC average-state variablesDeclaring
qv_avgalongsiderho_avg,H_avg, andgamma_avgin HLLC matches its role as a reference-energy average for the EOS. This is the right place to store it for average sound‑speed computations.
2003-2004: ME3 (model_eqns == 3) GPU private list includes qv_avgIn the ME3 branch, the GPU
privatelist now carriesqv_L,qv_R, andqv_avg, which are used in the energy and sound‑speed computations. This prevents unintended sharing of these scalars across threads and is consistent with their usage.
2010-2077: ME3: qv_L/qv_R computed consistently with density and EOSFor the 6‑equation ME3 branch:
qv_L/qv_Rare initialized to zero and then accumulated assum(alpha_rho * qvs)inside the same loop that formsrho,gamma, andpi_inf.- E and H include
qv_*viaE_* = gamma_* p + pi_inf_* + 0.5 ρ v² + qv_*, which is consistent with the stiffened‑gas form used elsewhere.Assuming
@:compute_average_state()takes care ofqv_avg, these changes look physically and numerically consistent.
2160-2170: ME3 average-state speed-of-sound uses qv_avgThe left/right sound speeds now use
qv_L/qv_R, andc_avgusesqv_avg. This aligns the average-state wave‑speed estimate with the updated SG EOS. Just ensure@:compute_average_state()setsqv_avgbefore this call for all ME3 cases.
2416-2417: ME4 GPU private list updated with qv and averagesIn the ME4 branch, the GPU
privatelist now containsqv_L,qv_R, andqv_avgalongsiderho_avg,H_avg,gamma_avg, etc. This matches subsequent use in E/H and speed-of-sound and is necessary for thread safety on GPU.
2452-2463: ME4: qv_L/qv_R computed analogously to other mixture propertiesHere
qv_L/qv_Rare computed as mixture‑weighted sumssum(alpha_rho * qvs)in the same loops that computerho,gamma, andpi_inf. This is consistent and should keep qv proportional to partial mass contributions.
2477-2487: ME4 sound-speed calls correctly pass qv for all statesIn ME4:
- Left/right calls to
s_compute_speed_of_soundpassqv_Landqv_R.- The average‑state call passes
qv_avg.This ensures wave speeds respect the reference-energy offset for both side and averaged states. As with other branches, correctness depends on
qv_avgbeing initialized by the average‑state macro.
2659-2661: bubbles_euler (model_eqns == 2) GPU private list now includes qv and averagesFor the 2‑equation bubbles‑euler branch, the
privatelist now carriesqv_L,qv_R,qv_avg, andc_avg. This matches their usage in speed‑of‑sound and average‑state computations and avoids shared‑state issues in the GPU loop.
2863-2873: bubbles_euler ME2: average sound speed now properly includes qv_avgLeft/right sound speeds use
qv_L/qv_R, and the average call now usesqv_avg. With the new initialization ofqv_avg, average‑state wave speeds are now consistent with the stiffened‑gas EOS for this bubbles‑euler branch.
3096-3097: 5-equation HLLC branch GPU private list updated with qv_avgFor the 5‑equation model, the GPU
privatelist now includesqv_L,qv_R, andqv_avgamong the other state and average variables. This matches later usage in E/H and sound‑speed computations.
3231-3237: 5-equation HLLC: non-chemistry energy uses qv_L/qv_R consistentlyIn the non‑chemistry case, E is formed as:
E_L = gamma_L*pres_L + pi_inf_L + 0.5*rho_L*vel_L_rms + qv_LE_R = gamma_R*pres_R + pi_inf_R + 0.5*rho_R*vel_R_rms + qv_Rand then H is
(E + p)/rho. This matches the intended SG EOS with reference energy and ensures enthalpy and sound speed see the same offset.
3300-3310: 5-equation HLLC: all speed-of-sound calls extended with qvFor the 5‑equation branch:
- Left/right calls pass
qv_Landqv_R.- The average call uses
qv_avgalong withrho_avg,gamma_avg, andH_avg.Given
qv_avgis part of the average‑state data, this keeps the characteristic speeds consistent with the updated SG EOS.
3755-3757: HLLD MHD: sound speed now uses qv%L/qv%RIn the HLLD solver, sound speeds for the MHD fast‑wave calculation now pass
qv%Landqv%Rintos_compute_speed_of_sound. Sinceqvis built alongsiderho,gamma, andpi_inf, this change correctly threads the reference energy into the MHD wave‑speed estimate as well.
2 participants
User description
User description
Description
This PR is to expand the SG EoS to its general form.
Such a change is needed for the PC model to work on both 5- and 6-equation models. The major difference is to include the 'q' parameter from the thermal SG EoS
p = (gamma - 1) * rho *(e - q) - gamma * ps_inf
which is also needed to calculate the speed of sound for the 5-equation model
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?
Comparison between the pressure behaviour without (black) and with (red) the addition of 'q' into the calculation of the speed of sound.
Test Configuration:
Checklist
They run to completion and demonstrate "interesting physics"
./mfc.sh formatbefore committing my codeIf your code changes any code source files (anything in
src/simulation)To make sure the code is performing as expected on GPU devices, I have:
nvtxranges so that they can be identified in profiles./mfc.sh run XXXX --gpu -t simulation --nsys, and have attached the output file (.nsys-rep) and plain text results to this PR./mfc.sh run XXXX --gpu -t simulation --rsys --hip-trace, and have attached the output file and plain text results to this PR.PR Type
Bug fix, Enhancement
Description
Expand Stiffened Gas (SG) equation of state to general form
qvparameter (internal energy reference value) in speed of sound calculationsSimplify 3-equation model speed of sound calculation
Update all speed of sound function calls across codebase
qvparameter tos_compute_speed_of_soundsubroutine signatureqvthrough enthalpy and data output modulesDiagram Walkthrough
File Walkthrough
m_variables_conversion.fpp
Add qv parameter and simplify speed of soundsrc/common/m_variables_conversion.fpp
qvparameter tos_compute_speed_of_soundsubroutine signaturevectorized sum
qv/rhoterm in speed of soundformula
m_data_output.fpp
Include qv in energy data output calculationssrc/post_process/m_data_output.fpp
qvvariable declaration and initializationqvas weighted sum of fluid reference energiesqvtermqvtos_compute_speed_of_soundfunction callm_start_up.fpp
Update speed of sound call with qv fieldsrc/post_process/m_start_up.fpp
qv_sffield tos_compute_speed_of_soundfunction callm_cbc.fpp
Add qv to CBC speed of sound calculationsrc/simulation/m_cbc.fpp
qvparameter tos_compute_speed_of_soundfunction callm_data_output.fpp
Propagate qv through stability criteria computationsrc/simulation/m_data_output.fpp
qvvariable to private list in GPU parallel loops_compute_enthalpycall to compute and returnqvqvto alls_compute_speed_of_soundfunction callsm_riemann_solvers.fpp
Add qv to all Riemann solver speed of sound callssrc/simulation/m_riemann_solvers.fpp
qv_avgvariable declaration for average state computations_compute_speed_of_soundcalls to includeqvparametersqv_L,qv_R, andqv_avgto speed of sound calculationsm_sim_helpers.fpp
Make qv output parameter in enthalpy computationsrc/simulation/m_sim_helpers.fpp
qvas output parameter tos_compute_enthalpysubroutineqvfrom local variable to subroutine outputm_time_steppers.fpp
Include qv in time stepper CFL computationsrc/simulation/m_time_steppers.fpp
qvvariable declaration with documentationqvto private list in GPU parallel loops_compute_enthalpycalls to computeqvqvtos_compute_speed_of_soundfunction callCodeAnt-AI Description
Account for SG q reference in sound speed and diagnostics
What Changed
Impact
✅ Correct sound speed for SG 5/6-equation waves✅ Accurate energy and Mach diagnostics with SG q reference✅ Stable timestep decisions based on q-aware enthalpy💡 Usage Guide
Checking Your Pull Request
Every time you make a pull request, our system automatically looks through it. We check for security issues, mistakes in how you're setting up your infrastructure, and common code problems. We do this to make sure your changes are solid and won't cause any trouble later.
Talking to CodeAnt AI
Got a question or need a hand with something in your pull request? You can easily get in touch with CodeAnt AI right here. Just type the following in a comment on your pull request, and replace "Your question here" with whatever you want to ask:
This lets you have a chat with CodeAnt AI about your pull request, making it easier to understand and improve your code.
Example
Preserve Org Learnings with CodeAnt
You can record team preferences so CodeAnt AI applies them in future reviews. Reply directly to the specific CodeAnt AI suggestion (in the same thread) and replace "Your feedback here" with your input:
This helps CodeAnt AI learn and adapt to your team's coding style and standards.
Example
Retrigger review
Ask CodeAnt AI to review the PR again, by typing:
Check Your Repository Health
To analyze the health of your code repository, visit our dashboard at https://app.codeant.ai. This tool helps you identify potential issues and areas for improvement in your codebase, ensuring your repository maintains high standards of code health.
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.