Skip to content

IGR integration#1615

Draft
StevenXue98 wants to merge 21 commits into
MFlowCode:masterfrom
StevenXue98:igr-integration
Draft

IGR integration#1615
StevenXue98 wants to merge 21 commits into
MFlowCode:masterfrom
StevenXue98:igr-integration

Conversation

@StevenXue98

@StevenXue98 StevenXue98 commented Jun 22, 2026

Copy link
Copy Markdown

Description

This PR integrates IGR with more of the shared RHS/Riemann infrastructure while preserving the existing IGR numerical behavior. This is an intermediate integration milestone, not the final IGR architecture. It intentionally preserves the current IGR reconstruction/Sigma/viscous-source behavior while moving RHS accumulation, RK update behavior, state layout, and LF Riemann dispatch toward shared MFC infrastructure.

Main changes:

  • Store the final volume fraction for IGR so the IGR state layout matches the normal model-equations layout more closely.
  • Route IGR face states through the shared s_riemann_solver path using the LF Riemann solver.
  • Use shared RHS accumulation helpers for IGR:
    • s_compute_advection_source_term
    • s_compute_additional_physics_rhs
  • Split the old fused s_igr_riemann_solver behavior into smaller IGR-specific routines called from the shared RHS path:
    • s_igr_compute_jac_rhs
    • s_igr_reconstruct_cell_boundary_values
    • s_igr_get_viscous
    • s_igr_correct_lf_fluxes
    • s_igr_sigma
  • Reuse stored IGR velocity-gradient arrays for viscous source-flux construction instead of recomputing those derivatives inside reconstruction.
  • Preserve legacy IGR behavior where needed:
    • skip the normal LF MPP limiter for IGR, since it was not part of the old IGR path
    • skip normal LF viscous-source construction for IGR, since IGR fills those source-flux slots itself
  • Remove the special padded rhs_vf allocation previously needed by IGR-specific direct RHS updates.

IGR-specific reconstruction, Sigma correction, and viscous source-flux construction are still present in m_igr.fpp. Subroutine s_igr_riemann_solver is also still present in m_igr.fpp, which explains the increased line count.

Deferred follow-up work:

  • evaluate whether IGR can switch from reconstruct-conservative-then-convert to the normal convert-to-primitive-then-reconstruct order
  • if numerically acceptable, route IGR face reconstruction through the standard reconstruction path
  • move IGR viscous source-flux construction closer to the normal viscous pipeline
  • revisit compatibility with MHD and other currently prohibited physics

Type of change

  • Refactor

Testing

Local testing:

./mfc.sh test --add-new-variables --only IGR
./mfc.sh test --add-new-variables --from F926448C --to F926448C
./mfc.sh test --add-new-variables --from 18A71AB2 --to 18A71AB2
./mfc.sh test --add-new-variables --from 43B5FEBD --to 43B5FEBD
./mfc.sh test --add-new-variables --from E49EF7B6 --to E49EF7B6
./mfc.sh test --only IGR

Additional local comparison/debug checks used during development:

python3 local_check_igr_full_alpha.py --default-igr
python3 local_igr_diagnostics/scripts/compare_igr_packs.py --default-igr

The IGR golden files were updated with --add-new-variables to include the newly stored final volume fraction. Existing golden entries were preserved, and the added final-alpha fields satisfy the expected volume-fraction sum checks, which is verified by the local Python scripts. The updated IGR tests match the previous conserved-variable outputs to machine-level roundoff, aside from the newly added final volume-fraction fields.

Checklist

Check these like this [x] to indicate which of the below applies.

  • I added or updated tests for new behavior
  • I updated documentation if user-facing behavior changed

See the developer guide for full coding standards.

GPU changes (expand if you modified src/simulation/)
  • GPU results match CPU results
  • Tested on NVIDIA GPU or AMD GPU
  • Verified CPU only

AI code reviews

Reviews are not retriggered automatically. To request a review, comment on the PR:

  • @claude full review — Claude full review (also triggers on PR open/reopen/ready)
  • Or add label claude-full-review — Claude full review via label

@StevenXue98 StevenXue98 changed the title Igr integration IGR integration Jun 23, 2026
@StevenXue98 StevenXue98 marked this pull request as ready for review June 23, 2026 22:05
@StevenXue98 StevenXue98 requested a review from sbryngelson as a code owner June 23, 2026 22:05
@codecov

codecov Bot commented Jun 24, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 82.44681% with 165 lines in your changes missing coverage. Please review.
✅ Project coverage is 57.89%. Comparing base (f4ab1aa) to head (267de82).
⚠️ Report is 1 commits behind head on master.

Files with missing lines Patch % Lines
src/simulation/m_rhs.fpp 66.31% 30 Missing and 34 partials ⚠️
src/simulation/m_igr.fpp 91.60% 0 Missing and 57 partials ⚠️
src/simulation/m_time_steppers.fpp 31.11% 15 Missing and 16 partials ⚠️
src/simulation/m_riemann_state.fpp 11.11% 6 Missing and 2 partials ⚠️
src/simulation/m_riemann_solver_lf.fpp 0.00% 0 Missing and 3 partials ⚠️
src/simulation/m_riemann_solvers.fpp 0.00% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1615      +/-   ##
==========================================
- Coverage   60.43%   57.89%   -2.55%     
==========================================
  Files          83       83              
  Lines       19871    20570     +699     
  Branches     2956     3009      +53     
==========================================
- Hits        12010    11908     -102     
- Misses       5860     6681     +821     
+ Partials     2001     1981      -20     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@StevenXue98 StevenXue98 marked this pull request as draft June 24, 2026 01:01
@StevenXue98

StevenXue98 commented Jul 3, 2026

Copy link
Copy Markdown
Author

I ran some local performance comparisons against the legacy IGR path.

Representative CPU results:

18A71AB2, 50 steps, 100 runs:

  • runtime: 3.48s refactor vs 2.74s legacy (~1.27x)
  • peak RSS: 71.6 MB refactor vs 57.4 MB legacy (~1.25x)

18A71AB2, 500 steps, 10 runs:

  • runtime: 29.64s refactor vs 22.61s legacy (~1.31x)
  • peak RSS: 71.6 MB refactor vs 57.3 MB legacy (~1.25x)

18A71AB2, larger grid, ~100^3, 20 steps, 3 runs:

  • runtime: 58.54s refactor vs 55.31s legacy (~1.06x)
  • peak RSS: 848.2 MB refactor vs 276.5 MB legacy (~3.07x)

@StevenXue98 StevenXue98 marked this pull request as ready for review July 3, 2026 22:31
@sbryngelson sbryngelson added the claude-full-review Trigger Claude Code review label Jul 6, 2026
@github-actions

github-actions Bot commented Jul 6, 2026

Copy link
Copy Markdown

Claude Code Review

Head SHA: 267de82

Files changed:

  • 13
  • src/common/include/3dHardcodedIC.fpp
  • src/common/m_global_parameters_common.fpp
  • src/common/m_variables_conversion.fpp
  • src/pre_process/m_assign_variables.fpp
  • src/simulation/m_igr.fpp
  • src/simulation/m_rhs.fpp
  • src/simulation/m_riemann_solver_lf.fpp
  • src/simulation/m_riemann_solvers.fpp
  • src/simulation/m_riemann_state.fpp
  • src/simulation/m_start_up.fpp
    (+2 more: src/simulation/m_time_steppers.fpp, toolchain/mfc/case_validator.py, toolchain/mfc/test/cases.py)

Findings:

  • src/simulation/m_igr.fpp — new s_igr_reconstruct_cell_boundary_values stores the L/R face states with the offset convention backwards, so the shared LF Riemann solver reads mismatched left/right states. Throughout the new subroutine (x/y/z blocks, e.g. qL_rsx_vf(j + 1, k, l, i) = alpha_rho_L(i) / qR_rsx_vf(j, k, l, i) = alpha_rho_R(i)), the reconstructed L value for the face computed at stencil index j is written to slot j + 1, while R is written unshifted to slot j. This is backwards relative to the codebase's established convention: src/simulation/m_riemann_solver_lf.fpp's s_lf_riemann_solver (unchanged code, confirmed via its SF/STENCIL_VAR macro expansion) always reads qL_prim_rsx_vf(${SF('')}$, ...) (unshifted) for the left state and qR_prim_rsx_vf(${SF(' + 1')}$, ...) (shifted +1) for the right state — i.e. L should be stored unshifted and R shifted +1, the exact opposite of what the new reconstruction routine does. The newly added s_igr_correct_lf_fluxes (same file) also reads with the solver's convention (u_L = qL_vf(${SF('')}$,...), u_R = qR_vf(${SF(' + 1')}$,...)), so it is likewise fed mismatched data by the reconstruction routine. Only the also-new s_igr_get_viscous happens to read with the same (backwards) offset the reconstruction wrote, which is internally consistent but disagrees with the solver/flux-correction routines that consume the very same qL_rsx_vf/qR_rsx_vf arrays.
    • Failure scenario: for any IGR run (which per case_validator.py's new riemann_solver != 5 check must now go through this shared LF path), each call to s_riemann_solver/s_lf_riemann_solver and s_igr_correct_lf_fluxes combines the left state reconstructed for face j-1 with the right state reconstructed for face j+1 instead of the matching pair for face j. This silently corrupts the flux/advection-source computation at every cell face for every IGR simulation (density, momentum, energy, and volume-fraction fluxes alike), producing wrong results without any crash or diagnostic — the class of bug this project's regression tests exist to catch, but a systemic index-convention swap here would shift results uniformly rather than in a way that is easy to spot from a corner case.

@sbryngelson sbryngelson marked this pull request as draft July 7, 2026 14:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

claude-full-review Trigger Claude Code review

Development

Successfully merging this pull request may close these issues.

2 participants