Skip to content

Clean correct state subroutine - #1838

Draft
danieljvickers wants to merge 5 commits into
MFlowCode:masterfrom
danieljvickers:clean-correct-state-subroutine
Draft

Clean correct state subroutine#1838
danieljvickers wants to merge 5 commits into
MFlowCode:masterfrom
danieljvickers:clean-correct-state-subroutine

Conversation

@danieljvickers

@danieljvickers danieljvickers commented Sep 10, 2026

Copy link
Copy Markdown
Member

Since we are planning to extend the immersed boudnary method to include more features and EOS, I have moved out some of the subroutines involved in this calculation to clean up the s_ibm_correct_state subroutine. Mostly adding abstractions and making it clear where modifications for various EOS need to go.

I have a few branches open right now as I am still waiting for pull requests to go through. This will heavily conflict with my current debug branch that is still uncovering new bugs. When that branch eventually gets merged, I will have to return and do similar abstraction for getting temperature and density and setting energy.

This is not a draft, but a full PR ready for merge. I am leaving it as a draft for now by request of @sbryngelson. I will let him determine when to open a PR

@github-actions

Copy link
Copy Markdown

Claude Code Review

Head SHA: 9ae77c5

Files changed:

  • 1
  • src/simulation/m_ibm.fpp

Findings:

  • src/simulation/m_ibm.fpp (new s_compute_ghost_point_pressure, ~line 166-172): the case default branch for state-dependent EOS (Mie-Gruneisen/JWL/Vinet) only calls s_mpi_abort under #ifndef MFC_GPU. On a GPU build, hitting this branch (moving IB + a fluid using a non-ideal/non-stiffened-gas EOS) silently skips the pressure contribution for that fluid instead of erroring — pres_GP stays at its 0._wp initializer contribution for that species, and if every fluid uses a state-dependent EOS the ghost-point pressure is silently set to 0. Since src/simulation/ is the only GPU-accelerated target and this guard exists specifically to prevent an unimplemented/incorrect physics path from running, the CPU-only abort leaves GPU runs to silently produce wrong energy/pressure fields for this combination rather than failing loudly, which is the exact "silent wrong answer" failure mode this codebase treats as most severe.

@danieljvickers

Copy link
Copy Markdown
Member Author

@sbryngelson This is read for PR

@sbryngelson

Copy link
Copy Markdown
Member

Reviewed 5c48618. The velocity extraction is a faithful move - s_compute_ghost_point_velocity reproduces the slip/no-slip branches, the rotation superposition and the v_blow block statement for statement, in the same order. The other two cleanups are behaviour-preserving too: physical_loc = [x_cc, y_cc, 0]; if (num_dims == 3) ... matches the old if (p > 0) form, hoisting radial_vector is required for it to be defined at the call and it is only read under moving_ibm /= 0, and accumulating the pressure into a real(wp) local instead of into q_prim_vf(E)%sf removes a per-iteration round trip through stp storage - identical by default and under --single, strictly better under --mixed.

One thing is not behaviour-preserving.

The new select case (eoss(q)) silently zeroes the moving-IB ghost pressure on GPU

src/simulation/m_ibm.fpp:160-172. The old body applied the correction unconditionally for q = 1, num_fluids. The new one wraps it in select case (eoss(q)) with case (eos_ideal_gas, eos_stiffened_gas), and a case default whose entire content is:

#ifndef MFC_GPU
    call s_mpi_abort('s_compute_ghost_point_pressure: moving IB pressure correction is only ' &
                     & // 'implemented for eos_ideal_gas/eos_stiffened_gas')
#endif

So with moving_ibm >= 2 and any fluid whose eoss(q) is eos_mie_gruneisen, eos_jwl or eos_vinet:

  • CPU: hard abort mid-solve.
  • GPU: the case default is empty, pres_GP keeps its 0._wp initialisation for that fluid, and q_prim_vf(eqn_idx%E)%sf(j,k,l) is set to zero (or a partial sum) at every ghost point. No warning, no NaN - a zero-pressure IB surface.

That is a CPU/GPU divergence producing a silent wrong answer on the GPU path, which CLAUDE.md ranks first among review priorities. There is no input-time guard: nothing in src/*/m_checker*.fpp or case_validator.py ties moving_ibm to eos.

Per the project's own rule that validation belongs in case_validator.py and the m_checker* files rather than scattered through the solver, the restriction wants to be a startup check rejecting moving_ibm > 1 together with a state-dependent EOS, leaving the kernel unconditional as it was. As written the abort is unreachable in exactly the configuration where it matters.

Minor

  • Dead locals. vel_norm_IP, v_blow_eff, norm, buf and rotation_velocity (declared around m_ibm.fpp:252-281) are no longer referenced in s_ibm_correct_state after the extraction, but remain declared and remain in the GPU_PARALLEL_LOOP private='[...]' clause. Harmless, but shrinking that routine was the point, and it keeps the private list honest - see the c_sum_Yi_Phi entry in the pitfalls file about private lists drifting from kernel bodies.
  • The two new routines are not in the module's private :: list (m_ibm.fpp:28-30). There is no bare private statement, so they default to public while every other internal helper here is explicitly listed.
  • s_compute_ghost_point_velocity adds a nesting level - a seq GPU_ROUTINE calling s_cross_product, where the call site used to be directly in the parallel-loop body. The argument is a fixed dimension(1:3), not a device-global bound, so ftn-7066 does not apply; still worth watching the Frontier CCE-ACC lane, since this module is where that class has bitten before.

CI is green (58 pass, 0 fail).

@danieljvickers

danieljvickers commented Sep 13, 2026

Copy link
Copy Markdown
Member Author

Addressed review comments by simply removing the select statement. But the sentiment of just prohibiting the case is correct, and I have added a prohibit to the python checker to compensate. Uneeded variables were removed, as is correct.

@github-actions

Copy link
Copy Markdown

Lines of Code

File Lines Diff
src/simulation/m_ibm.fpp 1267 +14
Directory Lines Diff
simulation 28051 +14
total 46276 +14

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants