Anchored Dual-pass HLLD for Hypoelasticity (+ HLLC and interface-consistent HLL)#1414
Anchored Dual-pass HLLD for Hypoelasticity (+ HLLC and interface-consistent HLL)#1414ChrisZYJ wants to merge 113 commits into
Conversation
|
And thanks @sbryngelson for the suggestions! I addressed the following cleanup items:
Also fixed the spelling CI failure: |
|
@sbryngelson If this looks good after the CI rerun, would it be possible to merge this PR soon? I’m happy to continue improving the code in follow-up PRs after this lands. The branch has already gone through several upstream re-merges and CI/review cleanup rounds, so merging the core implementation now would make future cleanup work smaller and easier to review. Thank you! |
|
I'm no longer merging messy/bloated/WIP code on the promise that it will be fixed later. I've been burned on this far too many times (perhaps not by you), and it only ratchets up, not down. Please clean your code of smells, refactor into nice helpers, and minimize SLOC while maintaining speed if you want the PR merged. Thanks. |
|
Okay. I'll refactor the solver into helpers according to your reviews, and minimize SLOC as you requested. Will post the changes here soon. |
…(identical codegen)
…e (identical codegen)
…hint on f_hlld_wave_zone
|
I've done the refactor. Here is what changed:
This removes 95 lines across the touched files. What is left in the main subroutine is dominated by the per-cell star-state algebra, which appears only once, so there is no duplication left to remove. Extracting it behind an interface would take ~50 scalar arguments or regrouping the privatized locals into derived types, and either would change register allocation on the GPU kernel. Since the goal was fewer lines at unchanged speed, that block stays inline, and the helper extractions went where the interfaces are narrow. I've taken care to ensure that most changes do not affect the generated code, and that they have minimal impact on performance. Also merged in the current master. |
|
One note on CI. The last two runs each failed a single Frontier job at the Fetch Dependencies step, before anything gets built or tested (gpu-omp 1/2, then gpu-acc 2/2). Both failures show the same error on the same runner, frontier-5: Maybe a corrupted uv cache entry on that runner's node? Could you help me re-run the one failed job? All the tests should pass. |
Self-hosted Frontier/Frontier-AMD matrix legs (acc/omp/cpu x shards) run their "Fetch Dependencies" step directly on the same login node as the same OS user, all pointed at the same UV_CACHE_DIR (introduced in #1385 to dodge NFS file-lock errors on ~/.cache/uv). uv's own cache lock guards individual entries, but concurrent installs from separate uv processes can still race while one extracts/prunes the shared archive-v0 store, leaving a corrupted entry behind (e.g. a missing dist-info METADATA file) that fails every subsequent install until the cache is cleared by hand -- as happened on PR #1414's Frontier gpu-acc [2/2] job. Serialize the actual `uv pip install` call with flock so only one process touches a given cache dir at a time, while keeping the cache itself shared and warm across runs.
|
Addresses Copilot review on #1630, plus a live recurrence caught on this PR's own CI run (job 85133634699, "Frontier (AMD) cpu [1/2]"): - UV_LOCK_DIR's guard only checked -w, so a writable non-directory TMPDIR would pass and then get used as a directory prefix, breaking flock. Add a -d check alongside -w, per Copilot's suggestion. - That same CI run hit a *new* corruption symptom ("The wheel is invalid: Missing .dist-info directory" for pandas) on the same physical login node (login05) as the original incident on #1414, even with the new lock in place. Root cause: a cache entry corrupted before the lock existed (or by any other cause) just fails forever until someone manually clears it -- which is exactly what had happened here; login05's ~1.2GiB cache had never actually been cleared (an earlier `uv cache clean` in this investigation was run from a different login node's session and never touched login05). Since self-hosted runners are spread across login nodes we can't all individually SSH into and inspect every time this happens, make the script self-heal instead: on install failure, clear the uv cache and retry once before giving up.
# Conflicts: # toolchain/mfc/test/cases.py
# Conflicts: # src/simulation/m_riemann_solver_hll.fpp # src/simulation/m_riemann_solver_hllc.fpp # src/simulation/m_riemann_solver_lf.fpp # src/simulation/m_riemann_state.fpp
|
Hi @ChrisZYJ — #1572 (Phase-3 Riemann hot-path decomposition) restructured/removed the exact solver code this PR builds on, which is what put I resolved the merge of current
Its first parent is your current head ( git fetch https://github.com/sbryngelson/MFC.git pr1414-hypo-hlld-master-merge
git checkout hypo_hlld
git merge --ff-only FETCH_HEAD
git push(Or just The core tension#1572 deleted the HLLC/LF hypoelastic-energy blocks as dead code, justified by Choices made (10 conflict hunks)
Two things worth your eyes
Happy to adjust any of these if you'd prefer a different call. cc @sbryngelson |
…er (drop dead six-eqn block, unused Re_max); fix HLL right-state damage index
…n hypo feature constraints with validator parity, constraint tests, and damage/three-fluid regressions
…near-zero-G elastic energy guards; regenerate the 12 affected hypo goldens
Mirror the Python validator's model and solver restrictions with Fortran PROHIBIT checks, use named solver constants in the touched checker logic, and remove the one-off parameter pytest.
Description
Adds:
Key Design Choices
Separate HLLD Riemann Solvers
At a glance it might be tempting to combine HLLD MHD with dual-pass hypoelasticity HLLD, but keeping them separate makes the code cleaner and much easier to maintain because:
Riemann Source Terms
For the non-conservative terms, unlike the usual governing equations that only need div U i.e. du/dx, dv/dy, dw/dz (alpha div U, K div U, etc.), Hypoelasticity has cross terms like du/dy, so we must also pass those Riemann-consistent traces from Riemann solver to the rhs. (The old Hypoelasticity code with the HLL Riemann solver uses finite difference for non-conservative rhs, which provides enough stability given that HLL smears the interface immediately, so there wasn't a need to pass the du/dy traces before this PR. But that does not work for HLLC/HLLD for Hypoelasticity.)
Also grouped/named the condition branches (with lots of comments within the code):
adv_src_alpha_ifaceflux_src_n(dir)%vf(j_adv)= per-fluidnc_iface_vel_n(dir)%vf(1)adv_src_vel_ifaceflux_src_n(dir)%vf(adv\%beg)= sharedflux_src_nslot (alreadyadv_src_noneThe derivations, meanings, and usage of the Riemann source variables are not straightforward. I've added some hopefully very helpful notes in
misc/dev_notesfor future developers (or AI agents; directing them to my notes should help them make fewer mistakes with the source terms) in terms of the understanding and derivations for the HLL/HLLC non-conservative fluxes, and their variable mapping for Riemann solvers and RHS.Backwards Compatibiilty
Type of change
Testing
All tests passed locally on CPU and Nvidia GPU, and on Frontier.
Smooth Eigenmode Convergence
Checklist
AI code reviews
Reviews are not triggered automatically. To request a review, comment on the PR:
@coderabbitai review— incremental review (new changes only)@coderabbitai full review— full review from scratch/review— Qodo review/improve— Qodo code suggestions@claude full review— Claude full review (also triggers on PR open/reopen/ready)claude-full-review— Claude full review via label