Conversation
Claude Code ReviewHead SHA: 202d7da Summary
Findings[HIGH] Missing Fortran-side runtime validation for [HIGH] [MEDIUM] Backward compatibility break: [MEDIUM] Removed constraint on [MEDIUM] [LOW] Hardcoded [LOW] Redundant precision cast [LOW] MTHINC not actually exercised in some new tests |
ⓘ You are approaching your monthly quota for Qodo. Upgrade your plan Review Summary by QodoAdd MTHINC interface compression and refactor THINC functionality
WalkthroughsDescription• Implements MTHINC (multi-dimensional THINC) interface compression for volume fraction sharpening alongside existing THINC support • Introduces new m_thinc module with helper functions for stable logarithmic-cosine computation, 1D THINC integrals, and multi-dimensional volume integrals using Gaussian quadrature • Converts int_comp parameter from logical to integer type with three states: 0=off, 1=THINC, 2=MTHINC • Integrates THINC/MTHINC compression into RHS computation with Newton iteration for interface position parameter solving • Refactors interface compression functionality out of MUSCL module into dedicated m_thinc module • Updates MPI communication to handle new integer parameter type • Adds comprehensive test cases for MTHINC in 2D/3D configurations with WENO reconstruction • Updates all example cases and parameter definitions to use new integer enum values • Includes performance validation showing ~3% overhead for MTHINC vs ~1% for THINC • Updates documentation and validation logic for the new parameter type and options Diagramflowchart LR
A["int_comp: logical<br/>to integer"] --> B["m_thinc module<br/>THINC + MTHINC"]
B --> C["RHS computation<br/>with compression"]
B --> D["Startup initialization<br/>and finalization"]
C --> E["Test cases<br/>2D/3D MTHINC"]
A --> F["MPI broadcast<br/>parameter update"]
A --> G["Parameter validation<br/>0/1/2 values"]
File Changes1. src/simulation/m_thinc.fpp
|
Code Review by Qodo
1. int_comp absent on device
|
Claude Code ReviewHead SHA: Key source files:
Summary
Findings[Medium] Missing Fortran runtime check for
@:PROHIBIT(int_comp < 0 .or. int_comp > 2, "int_comp must be 0 (off), 1 (THINC), or 2 (MTHINC)")The Python validator catches this at case-setup time, but the Fortran check is the convention for parameters with constrained integer values. [Medium] MTHINC silently degrades to 1-D THINC for 1-D cases — When
Mathematically this is fine (MTHINC reduces to THINC in 1-D), but the user gets THINC behaviour when requesting MTHINC with no diagnostic. The old Python constraint that required [Low] MTHINC skips monotonicity check — The 1-D THINC path guards with [Low] Two-fluid assumption is undocumented — Both THINC and MTHINC update only [Positive] Correct THINC bug fix The old right-reconstruction in vR...(contxb) = vL...(contxb) / vL...(advxb) * aTHINC ! Bug: advxb already overwrittenThe new code in [Positive] Architecture improvement Centralising interface compression into |
There was a problem hiding this comment.
Pull request overview
This PR introduces a dedicated THINC/MTHINC interface-compression module and migrates the int_comp setting from a logical flag to an integer selector, while updating tests/docs/tooling accordingly.
Changes:
- Added
src/simulation/m_thinc.fppimplementing THINC (int_comp=1) and MTHINC (int_comp=2) and wired it into the reconstruction flow inm_rhs. - Replaced the old MUSCL/WENO-local interface-compression hooks by centralizing compression in RHS reconstruction; removed the MUSCL
s_interface_compressionimplementation. - Updated toolchain parameter typing/validation and adjusted examples/docs/tests to use integer
int_comp.
Reviewed changes
Copilot reviewed 42 out of 81 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
src/simulation/m_thinc.fpp |
New THINC/MTHINC implementation (including MTHINC normal computation and Newton solve). |
src/simulation/m_rhs.fpp |
Calls MTHINC normal computation pre-split; runs compression post-reconstruction with NVTX ranges. |
src/simulation/m_muscl.fpp |
Removes s_interface_compression and MUSCL-side compression call. |
src/simulation/m_weno.fpp |
Removes MUSCL import and WENO-side compression call. |
src/simulation/m_start_up.fpp |
Initializes/finalizes the new THINC module when int_comp > 0. |
src/simulation/m_global_parameters.fpp |
Changes int_comp from logical to integer with updated defaults/comments. |
src/simulation/m_mpi_proxy.fpp |
Broadcasts int_comp as an integer instead of logical. |
toolchain/mfc/params/definitions.py |
Registers int_comp as INT instead of LOG. |
toolchain/mfc/case_validator.py |
Validates int_comp is in {0,1,2}. |
toolchain/mfc/test/cases.py |
Expands generated test matrix to include int_comp values {0,1,2}. |
toolchain/mfc/params/descriptions.py |
Updates parameter description to document 0/1/2 meanings. |
docs/documentation/case.md |
Updates docs to describe integer int_comp and mentions MTHINC. |
docs/module_categories.json |
Adds m_thinc to documentation module grouping. |
examples/*/case.py |
Updates examples from "T" to 1 for THINC. |
tests/*/golden-metadata.txt |
Updates/adds golden metadata for the new/updated test set. |
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
📝 WalkthroughWalkthroughThis pull request refactors interface compression from a boolean flag into an integer enumeration with three states: 0 (off), 1 (THINC), and 2 (MTHINC). The implementation moves compression logic from the MUSCL module into a dedicated new THINC module. Interface compression is now integrated into RHS calculations and invoked during variable reconstruction. Documentation, parameter definitions, validation logic, and example configurations are updated to reflect the new integer-based parameter type and semantic values. Test cases are expanded to exercise the new enumeration states. 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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: 9
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 411ddfaf-e410-411e-a688-203d6a1caf71
📒 Files selected for processing (81)
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/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/09195EF4/golden-metadata.txttests/09195EF4/golden.txttests/0A362971/golden-metadata.txttests/0A362971/golden.txttests/11D609F6/golden-metadata.txttests/11D609F6/golden.txttests/1B300F28/golden-metadata.txttests/1F0C4A44/golden-metadata.txttests/1F0C4A44/golden.txttests/2C9844EF/golden-metadata.txttests/2C9844EF/golden.txttests/2D11A034/golden-metadata.txttests/2D11A034/golden.txttests/2E4AC44C/golden-metadata.txttests/2E4AC44C/golden.txttests/3ECA875A/golden-metadata.txttests/3ECA875A/golden.txttests/409B55DF/golden-metadata.txttests/409B55DF/golden.txttests/503EEFF7/golden-metadata.txttests/503EEFF7/golden.txttests/50EC2239/golden-metadata.txttests/50EC2239/golden.txttests/6102D509/golden-metadata.txttests/6102D509/golden.txttests/6775D763/golden-metadata.txttests/6775D763/golden.txttests/67C777D8/golden.txttests/7156050E/golden-metadata.txttests/7156050E/golden.txttests/845DC70C/golden-metadata.txttests/845DC70C/golden.txttests/885D5D8C/golden-metadata.txttests/885D5D8C/golden.txttests/8876692F/golden-metadata.txttests/8876692F/golden.txttests/8C42A56C/golden-metadata.txttests/8C42A56C/golden.txttests/A438B8CE/golden-metadata.txttests/A438B8CE/golden.txttests/A930AE61/golden-metadata.txttests/A930AE61/golden.txttests/AEE7A791/golden-metadata.txttests/AEE7A791/golden.txttests/B3C724B5/golden-metadata.txttests/B3C724B5/golden.txttests/B903D17E/golden-metadata.txttests/B903D17E/golden.txttests/C02D71EE/golden-metadata.txttests/C02D71EE/golden.txttests/C420EDF3/golden-metadata.txttests/C420EDF3/golden.txttests/CE35B602/golden-metadata.txttests/CE35B602/golden.txttests/E11FD23A/golden-metadata.txttests/E11FD23A/golden.txttests/F5FABAE9/golden-metadata.txttests/F5FABAE9/golden.txttests/FA4D8FEF/golden-metadata.txttests/FA4D8FEF/golden.txttests/FE9379D8/golden-metadata.txttests/FE9379D8/golden.txttoolchain/mfc/case_validator.pytoolchain/mfc/params/definitions.pytoolchain/mfc/params/descriptions.pytoolchain/mfc/test/cases.py
💤 Files with no reviewable changes (10)
- tests/FA4D8FEF/golden.txt
- tests/FA4D8FEF/golden-metadata.txt
- tests/2C9844EF/golden-metadata.txt
- src/simulation/m_weno.fpp
- tests/0A362971/golden.txt
- tests/1B300F28/golden-metadata.txt
- tests/8876692F/golden-metadata.txt
- tests/C02D71EE/golden-metadata.txt
- tests/50EC2239/golden-metadata.txt
- tests/0A362971/golden-metadata.txt
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #1303 +/- ##
==========================================
- Coverage 64.67% 64.32% -0.35%
==========================================
Files 70 71 +1
Lines 18251 18401 +150
Branches 1504 1523 +19
==========================================
+ Hits 11804 11837 +33
- Misses 5492 5604 +112
- Partials 955 960 +5 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Claude Code ReviewHead SHA: 85acf5a Files changed: 81 (key source files: Summary
Findings[Medium] if (int_comp == 2 .and. n > 0) then ! MTHINC guard
[Medium] Newton solver do iter = 1, 30
...
d = d - residual/dV
end doIf convergence fails (e.g., degenerate normal near a triple point or machine-epsilon gradient), the function returns an unclamped [Low] Removed [Low] Unused allocations for MTHINC in 1D [Info] Bug fix for #1181 confirmed correct ! BUG: vL_rs_vf_XYZ(advxb) already overwritten by left reconstruction above
vR_rs_vf_XYZ(contxb) = vL_rs_vf_XYZ(contxb) / vL_rs_vf_XYZ(advxb) * aTHINCNew code correctly saves Overall the implementation is well-structured. The |
sbryngelson
force-pushed
the
MTHINC
branch
2 times, most recently
from
2e0268f to
617f985
Compare
Claude Code ReviewIncremental review from: (first review) New findings:1. Critical — GPU coherence: All three variables are read inside
By contrast, the analogous scalar Fix: Add to $:GPU_DECLARE(create='[int_comp,ic_eps,ic_beta]')And add to $:GPU_UPDATE(device='[int_comp,ic_eps,ic_beta]')2. Moderate — MTHINC density reconstruction silently skips intermediate fluid components for In rho1 = v_vf(contxb)%sf(ix, iy, iz)/aC
rho2 = v_vf(contxe)%sf(ix, iy, iz)/(1._wp - aC)This implicitly assumes exactly 2 fluids. For There is no constraint in Fix: Either extend the density reconstruction to loop over all fluids from self.prohibit(int_comp > 0 and num_fluids > 2,
"int_comp (THINC/MTHINC) only supports num_fluids <= 2") |
|
<!-- claude-review: thread=primary; reviewed_sha=0ed0772ee8995e5752187f6d67d035a6bc49b556; mode=incremental --> |
|
@wilfonba can you re-run a few sanity checks to make sure this works after i made a bunch of changes? then i guess we can mark it ready to review, which ensures it works on frontier/phoenix/whatever. then merge. |
Yep. I'll send you a message and open it as ready to review when I've taken care of that. |
|
No description provided. |
sbryngelson
force-pushed
the
MTHINC
branch
4 times, most recently
from
416d0ad to
755fba3
Compare
sbryngelson
force-pushed
the
MTHINC
branch
2 times, most recently
from
878b943 to
3fe0129
Compare
|
PRs are ready to review (non-draft) when they are verified passing on amd gpu hardware + mpi (not the case here it seems). Setting from draft to ready to review triggers larger CI runs on every push. |
|
@sbryngelson this is verified passing on NVIDIA with OpenACC/MP and AMD Flang with OpenMP now. It also gives correct results with parallelization. The existing PR comments from the draft state have been addressed as well. |
|
Sheesh, seems like a weird edge case with compile-time parameters on CCE compilers is causing the issue with the tests. |
|
Marking this ready for review again since I've run a bunch of manual tests since the most recent changes |
Review Summary by QodoAdd MTHINC multi-dimensional interface compression and refactor THINC implementation
WalkthroughsDescription• Implements THINC (1D) and MTHINC (multi-dimensional) interface compression schemes for volume fraction sharpening in the simulation module • Converts int_comp parameter from logical to integer type (0=off, 1=THINC, 2=MTHINC) with comprehensive validation • Centralizes interface compression logic in new m_thinc module with helper functions for tanh-based profiles and Newton iteration for conservation constraints • Integrates MTHINC normal computation into RHS calculation pipeline and updates WENO/MUSCL modules to use centralized compression interface • Adds initialization and finalization routines for THINC module in startup sequence • Updates MPI broadcasts to handle new integer parameter type • Adds comprehensive test cases for THINC and MTHINC with dimension-aware testing (MTHINC only in 2D/3D) • Updates documentation and parameter definitions with clear descriptions of compression modes • Includes new golden test files and metadata for MTHINC validation • Demonstrates ~3% performance overhead for MTHINC and ~1% for THINC in 3D advection benchmarks Diagramflowchart LR
A["m_thinc module<br/>THINC/MTHINC compression"] --> B["m_rhs<br/>Compute normals"]
A --> C["m_weno<br/>Apply compression"]
A --> D["m_muscl<br/>Apply compression"]
E["m_global_parameters<br/>int_comp: 0/1/2"] --> A
E --> B
E --> C
E --> D
F["m_start_up<br/>Initialize/Finalize"] --> A
G["m_mpi_proxy<br/>Broadcast int_comp"] --> E
H["case_validator<br/>Validate int_comp"] --> E
File Changes1. src/simulation/m_thinc.fpp
|
Code Review by Qodo
1. Multi-fluid THINC corruption
|
|
@sbryngelson, I believe this is ready to merge. The failed benchmark shows good performance if you look at the generated comment. There was just some sort of error with the Slurm job. I can rerun it still if you want though. |
Description
This PR adds MTHINC to the code base and fixes the THINC bug in #1181. The existing golden files for THINC interface compression were preserved, and new golden files and tests were created for MTHINC.
Type of change
Testing
3D advection case with and without MTHINC.
test.mp4
2D advection with
int_comp = 1(THINC) 1 rank vs. 4 rank. Results are identicaltest.mp4
2D advection with
int_comp = 2(MTHINC) 1 rank vs. 4 rank. Results are identicaltest.mp4
3D advection with
int_comp = 2(MTHINC) 1 rank vs. 8 rank. Results are identicalI didn't bother making a video for this one, but the output was identical when compared in ParaView.
Performance tests in 3D
The following results are grind times for a 200^3 advection case run on 1 A100 with OpenACC. The overhead for MTHINC is ~3% while the overhead for THINC is ~1%. The grind times are:
(Updated 4/1/2026 after removing
parameterfrom constants inm_thinc.fppint_comp = 0: 0.87488 (No compression, baseline)int_comp = 1: 0.88406 (1%) (THINC compression)int_comp = 2: 0.90199 (3%) (MTHINC compression)--nsysresults withint_comp = 2This is the run time contribution for just the MTHINC routines.
Time (%) Total Time (ns) Instances Avg (ns) Med (ns) Min (ns) Max (ns) StdDev (ns) Name
Checklist
See the developer guide for full coding standards.
GPU changes (expand if you modified
src/simulation/)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