fix: correct MTHINC normals on non-uniform grids#1401
fix: correct MTHINC normals on non-uniform grids#1401sbryngelson wants to merge 41 commits intoMFlowCode:masterfrom
Conversation
- case.md: add muscl_eps row/description alongside int_comp (MTHINC version) - m_global_parameters.fpp: keep int_comp as integer (0/1/2), add muscl_eps; keep GPU_DECLARE; both GPU_UPDATEs - m_mpi_proxy.fpp: broadcast both int_comp and collision_model as integers - case_validator.py: keep recon_type validation from master; drop stale int_comp=boolean check (superseded by check_interface_compression)
…rface_tension f_thinc_integral_1d used log_cosh(a+h) - log_cosh(a-h) which suffers catastrophic cancellation in single precision when h (= beta*n_transverse/2) is small (interface nearly axis-aligned). Replace with the identity 2*atanh(tanh(a)*tanh(h)), which is algebraically equivalent and numerically stable at all precisions. Fixes the single-precision CI failure on the 2D and 3D MTHINC WENO tests (5126B21F, 4F3722DB). Also add validator rules prohibiting int_comp > 0 with viscous=T or surface_tension=T: the split reconstruction paths in m_rhs prevent s_thinc_compression from ever running in those cases, so permitting the combination would silently produce unsharpened results.
Claude Code ReviewHead SHA: cbabce3 Files changed:
Findings1.
|
ⓘ You've reached your Qodo monthly free-tier limit. Reviews pause until next month — upgrade your plan to continue now, or link your paid account if you already have one. |
📝 WalkthroughWalkthroughA new THINC interface compression module ( 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (3)
toolchain/mfc/lint_docs.py (1)
440-440: ⚡ Quick winAvoid making
check_interface_compressiona permanent docs-coverage blind spot.If this skip is temporary, please add a clear TODO/issue reference next to it so the PHYSICS_DOCS entry gets restored and coverage doesn’t silently regress.
As per coding guidelines, "Only comment on code that is within this PR’s diff (changed lines / files)."
toolchain/mfc/test/cases.py (2)
305-312: ⚡ Quick win
muscl_order=1coverage disappeared from the generated matrix.The
muscl_order == 1iteration now only pushes/pops the stack and never appends a case, so this helper stops exercising first-order MUSCL entirely. Please add a baselinedefine_case_d(...)before themuscl_order == 2specialization, or drop1from the loop if that mode is intentionally unsupported.
419-420: ⚡ Quick winThe new
int_compcoverage still misses the viscous/surface-tension path.
alter_int_comp(dimInfo)is only invoked before entering the two-fluidViscousbranch, so the generated suite never hits the full-reconstruction path this PR adds for viscous/surface-tension RHS handling. Adding at least one two-fluid viscous case withint_comp > 0would keep#1396covered.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 1833b390-712d-4929-a69c-fc3c67b6ea8a
📒 Files selected for processing (106)
docs/documentation/case.mddocs/module_categories.jsonexamples/1D_sodshocktube_muscl/case.pyexamples/2D_advection_muscl/case.pyexamples/2D_riemann_test_muscl/case.pyexamples/2D_shockdroplet_muscl/case.pyexamples/3D_rayleigh_taylor_muscl/case.pyexamples/3D_shockdroplet_muscl/case.pysrc/common/m_constants.fppsrc/simulation/m_global_parameters.fppsrc/simulation/m_mpi_proxy.fppsrc/simulation/m_muscl.fppsrc/simulation/m_rhs.fppsrc/simulation/m_start_up.fppsrc/simulation/m_thinc.fppsrc/simulation/m_weno.fpptests/0A362971/golden-metadata.txttests/0A362971/golden.txttests/238A7B1F/golden-metadata.txttests/238A7B1F/golden.txttests/35635326/golden-metadata.txttests/35635326/golden.txttests/3ECA875A/golden-metadata.txttests/3ECA875A/golden.txttests/4111B2ED/golden-metadata.txttests/4111B2ED/golden.txttests/4440D46B/golden.txttests/4C4F339C/golden-metadata.txttests/4C4F339C/golden.txttests/4E4FECA9/golden-metadata.txttests/4E4FECA9/golden.txttests/4F3722DB/golden-metadata.txttests/4F3722DB/golden.txttests/503EEFF7/golden-metadata.txttests/503EEFF7/golden.txttests/50EC2239/golden-metadata.txttests/50EC2239/golden.txttests/5126B21F/golden-metadata.txttests/5126B21F/golden.txttests/516A2067/golden-metadata.txttests/516A2067/golden.txttests/550E8BF5/golden-metadata.txttests/550E8BF5/golden.txttests/6077374F/golden.txttests/67C777D8/golden-metadata.txttests/67C777D8/golden.txttests/7A1719C6/golden-metadata.txttests/7A1719C6/golden.txttests/82DA2499/golden-metadata.txttests/82DA2499/golden.txttests/845DC70C/golden-metadata.txttests/845DC70C/golden.txttests/86E9A6D4/golden-metadata.txttests/86E9A6D4/golden.txttests/885D5D8C/golden-metadata.txttests/885D5D8C/golden.txttests/8876692F/golden-metadata.txttests/8876692F/golden.txttests/8C42A56C/golden-metadata.txttests/8C42A56C/golden.txttests/9EE432D4/golden-metadata.txttests/9EE432D4/golden.txttests/A089CE54/golden-metadata.txttests/A089CE54/golden.txttests/A930AE61/golden-metadata.txttests/A930AE61/golden.txttests/AD6ED274/golden-metadata.txttests/AD6ED274/golden.txttests/BAF51303/golden-metadata.txttests/BAF51303/golden.txttests/BDC2A773/golden-metadata.txttests/BDC2A773/golden.txttests/C02D71EE/golden-metadata.txttests/C02D71EE/golden.txttests/C4047138/golden-metadata.txttests/C605ECCC/golden-metadata.txttests/C7A6B609/golden.txttests/CE35B602/golden-metadata.txttests/CE35B602/golden.txttests/CF73B3B6/golden-metadata.txttests/CF73B3B6/golden.txttests/D49DCFC0/golden-metadata.txttests/D49DCFC0/golden.txttests/DC97B66D/golden-metadata.txttests/DC97B66D/golden.txttests/E11FD23A/golden-metadata.txttests/E11FD23A/golden.txttests/E3047A62/golden.txttests/E8979E4A/golden.txttests/EAD7CADE/golden-metadata.txttests/EBBC93BD/golden-metadata.txttests/EBBC93BD/golden.txttests/F1CF01C4/golden-metadata.txttests/F1CF01C4/golden.txttests/F44A293A/golden-metadata.txttests/F44A293A/golden.txttests/FA695993/golden-metadata.txttests/FA695993/golden.txttests/FE9379D8/golden-metadata.txttests/FE9379D8/golden.txttoolchain/mfc/case_validator.pytoolchain/mfc/lint_docs.pytoolchain/mfc/params/definitions.pytoolchain/mfc/params/descriptions.pytoolchain/mfc/params/registry.pytoolchain/mfc/test/cases.py
💤 Files with no reviewable changes (25)
- tests/86E9A6D4/golden-metadata.txt
- tests/8876692F/golden-metadata.txt
- tests/50EC2239/golden-metadata.txt
- tests/D49DCFC0/golden-metadata.txt
- tests/86E9A6D4/golden.txt
- tests/BDC2A773/golden.txt
- tests/238A7B1F/golden-metadata.txt
- tests/FA695993/golden-metadata.txt
- tests/67C777D8/golden-metadata.txt
- tests/F44A293A/golden-metadata.txt
- tests/550E8BF5/golden-metadata.txt
- tests/82DA2499/golden-metadata.txt
- tests/E3047A62/golden.txt
- tests/C4047138/golden-metadata.txt
- tests/C605ECCC/golden-metadata.txt
- tests/0A362971/golden-metadata.txt
- examples/2D_riemann_test_muscl/case.py
- tests/BDC2A773/golden-metadata.txt
- examples/1D_sodshocktube_muscl/case.py
- tests/C02D71EE/golden-metadata.txt
- tests/AD6ED274/golden-metadata.txt
- tests/BAF51303/golden-metadata.txt
- tests/EAD7CADE/golden-metadata.txt
- tests/9EE432D4/golden-metadata.txt
- tests/0A362971/golden.txt
| if (int_comp > 0 .and. v_size >= eqn_idx%adv%end) then | ||
| call nvtxStartRange("WENO-INTCOMP") | ||
| #:for MUSCL_DIR, XYZ in [(1, 'x'), (2, 'y'), (3, 'z')] | ||
| if (muscl_dir == ${MUSCL_DIR}$) then | ||
| call s_thinc_compression(v_rs_ws_${XYZ}$_muscl, vL_rs_vf_x, vL_rs_vf_y, vL_rs_vf_z, vR_rs_vf_x, vR_rs_vf_y, & | ||
| & vR_rs_vf_z, muscl_dir, is1_muscl, is2_muscl, is3_muscl) | ||
| end if | ||
| #:endfor | ||
| call nvtxEndRange() |
There was a problem hiding this comment.
int_comp can consume an uninitialized MUSCL workspace in first-order mode.
When muscl_order == 1, s_initialize_muscl() is skipped above, so v_rs_ws_*_muscl has not been populated before this block passes it to s_thinc_compression(). That makes int_comp > 0 + first-order MUSCL undefined unless the combination is prohibited elsewhere. Either initialize the directional workspace for compressed runs too, or explicitly reject muscl_order == 1 when int_comp > 0.
Possible fix
- if (muscl_order /= 1 .or. dummy) then
+ if (muscl_order /= 1 .or. dummy .or. int_comp > 0) then
call s_initialize_muscl(v_vf, muscl_dir)
end if| !> @brief Stable difference: log_cosh(a+h) - log_cosh(a-h) = 2*atanh(tanh(a)*tanh(h)). Avoids catastrophic cancellation when h | ||
| !! is small relative to a. | ||
| function f_log_cosh_diff(a, h) result(res) | ||
|
|
||
| $:GPU_ROUTINE(parallelism='[seq]') | ||
| real(wp), intent(in) :: a, h | ||
| real(wp) :: res | ||
|
|
||
| res = 2._wp*atanh(tanh(a)*tanh(h)) | ||
|
|
There was a problem hiding this comment.
Clamp the atanh argument before calling it.
For large a or h, tanh(a) and tanh(h) can round to ±1, so the product can hit ±1 exactly and atanh returns infinities even though the original log_cosh(a+h) - log_cosh(a-h) is still finite. That will leak Inf/NaN into the THINC integrals on sharp interfaces.
Suggested fix
function f_log_cosh_diff(a, h) result(res)
$:GPU_ROUTINE(parallelism='[seq]')
real(wp), intent(in) :: a, h
- real(wp) :: res
+ real(wp) :: res, t
- res = 2._wp*atanh(tanh(a)*tanh(h))
+ t = tanh(a)*tanh(h)
+ t = max(-1._wp + epsilon(1._wp), min(1._wp - epsilon(1._wp), t))
+ res = 2._wp*atanh(t)
end function f_log_cosh_diff| if (int_comp > 0 .and. v_size >= eqn_idx%adv%end) then | ||
| call nvtxStartRange("WENO-INTCOMP") | ||
| #:for WENO_DIR, XYZ in [(1, 'x'), (2, 'y'), (3, 'z')] | ||
| if (weno_dir == ${WENO_DIR}$) then | ||
| call s_thinc_compression(v_rs_ws_${XYZ}$, vL_rs_vf_x, vL_rs_vf_y, vL_rs_vf_z, vR_rs_vf_x, vR_rs_vf_y, & | ||
| & vR_rs_vf_z, weno_dir, is1_weno, is2_weno, is3_weno) | ||
| end if | ||
| #:endfor | ||
| call nvtxEndRange() |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -e
echo "Calls and imports in src/simulation/m_weno.fpp:"
rg -n 'use m_nvtx|nvtxStartRange|nvtxEndRange' src/simulation/m_weno.fpp
echo
echo "NVTX procedure definitions in the codebase:"
fd -i 'm_nvtx.fpp' src | xargs -r rg -n 'subroutine[[:space:]]+nvtx(StartRange|EndRange)'Repository: MFlowCode/MFC
Length of output: 240
🏁 Script executed:
#!/bin/bash
set -e
echo "=== Looking for m_nvtx file ==="
fd -i 'm_nvtx' src/
echo
echo "=== Checking m_rhs.fpp for NVTX usage (as mentioned in review) ==="
rg -n 'use m_nvtx|nvtxStartRange|nvtxEndRange' src/simulation/m_rhs.fpp 2>/dev/null | head -20
echo
echo "=== Checking all files that call nvtxStartRange/nvtxEndRange ==="
rg -l 'nvtxStartRange|nvtxEndRange' src/
echo
echo "=== Check imports in files using NVTX ==="
for file in $(rg -l 'nvtxStartRange|nvtxEndRange' src/); do
echo "--- File: $file ---"
rg -n 'use m_|nvtxStartRange|nvtxEndRange' "$file" | head -30
doneRepository: MFlowCode/MFC
Length of output: 7302
Import m_nvtx before using the new range markers.
The NVTX instrumentation calls at lines 1387 and 1394 require the m_nvtx module, which is not imported in m_weno.fpp. This will cause a compile error. All other files that use nvtxStartRange and nvtxEndRange (e.g., m_rhs.fpp, m_time_steppers.fpp) explicitly include use m_nvtx.
Suggested fix
use m_variables_conversion
! $:USE_GPU_MODULE()
use m_mpi_proxy
+ use m_nvtx
use m_thinc, only: s_thinc_compression
Automated Code ReviewSummary: 1 critical issue, 2 important issues. Critical Issue1. Missing
Fix: Add Minor: the NVTX label Important Issues2.
res = 2._wp*atanh(tanh(a)*tanh(h))
Fix (option A): Clamp the t = tanh(a)*tanh(h)
res = 2._wp*atanh(sign(min(abs(t), 1._wp - epsilon(1._wp)), t))Fix (option B): Add 3. No Fortran-side
Fix: Add to @:PROHIBIT(recon_type == MUSCL_TYPE .and. muscl_order == 1 .and. int_comp /= 0, &
& "int_comp must be 0 when muscl_order = 1")Suggestions
Strengths
|
…ker for muscl_order==1+int_comp - m_muscl.fpp: add missing 'use m_nvtx' (compile error without it) - m_thinc.fpp: clamp tanh(a)*tanh(h) to (-1+eps, 1-eps) before atanh to prevent Inf/NaN on sharp interfaces (large ic_beta values) - m_checker.fpp: add runtime PROHIBIT for muscl_order==1 with int_comp>0 (uninitialized reconstruction workspace)
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #1401 +/- ##
==========================================
+ Coverage 64.75% 64.88% +0.12%
==========================================
Files 71 72 +1
Lines 18721 18883 +162
Branches 1551 1571 +20
==========================================
+ Hits 12123 12252 +129
- Misses 5640 5655 +15
- Partials 958 976 +18 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
2 participants
Summary
Builds on #1303 (MTHINC implementation). Fixes two bugs found during review:
int_compincompatible with viscous / surface-tension flowsFix 1: MTHINC normals on non-uniform grids (#1395)
The reference-space normal in
s_compute_mthinc_normalsused a uniform-grid assumption (* 0.5) that gives the wrong direction when cell widths vary. The correct weighting is:On uniform grids this ratio equals
0.5exactly, so all existing tests pass unchanged.Fix 2: int_comp with viscous / surface-tension flows (#1396)
int_comp > 0requires full (non-split) reconstruction, but the viscous and surface-tension RHS paths were calling split reconstruction, silently disabling interface compression. Fixed by routing those paths through the full reconstruction branch and adding a runtime@:PROHIBITguard.New regression test (
7A1719C6)A new golden-file test exercises Fix 1 on a non-uniform grid:
nr_x ≠ 0andnr_y ≠ 0) — only diagonal cells are sensitive to theΔx_jweighting correction; axis-aligned interfaces always normalize to±1regardless of scalingvel(1)=0.5advects the bubble so MTHINC reconstruction produces non-trivial fluxes; 129+ distinct values at t=50 confirm genuine interface evolutionTest plan
5126B21F,4E4FECA9,4F3722DB,4C4F339C)7A1719C6)./mfc.sh precheck -j 8passes./mfc.sh build -t simulation -j 8compiles cleanly