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. |
| int_comp = self.get('int_comp', 0) | ||
|
|
||
| self.prohibit(int_comp and recon_type != 2, | ||
| "int_comp (THINC interface compression) requires recon_type = 2 (MUSCL)") | ||
| self.prohibit(int_comp not in [0, 1, 2], | ||
| "int_comp must be 0 (off), 1 (THINC), or 2 (MTHINC)") |
There was a problem hiding this comment.
int_comp=2 (MTHINC) is described as multi-dimensional, but the Fortran implementation only runs the MTHINC path when n > 0 (i.e., 2D/3D). In 1D (n=0) it silently falls back to the 1D THINC branch, which makes int_comp=2 behave differently than documented and is easy to misconfigure. Consider adding a validation rule that prohibits int_comp == 2 unless the case is at least 2D (e.g., n > 0).
| if muscl_order == 1: | ||
| for int_comp in ["T", "F"]: | ||
| for int_comp in [0, 1, 2]: | ||
| cases.append(define_case_d(stack, f"int_comp={int_comp}", {'int_comp': int_comp})) | ||
| elif muscl_order == 2: | ||
| for int_comp in ["T", "F"]: | ||
| for int_comp in [0, 1, 2]: | ||
| stack.push(f"int_comp={int_comp}", {'int_comp': int_comp}) | ||
| cases.append(define_case_d(stack, f"muscl_lim=1", {'muscl_lim': 1})) |
There was a problem hiding this comment.
This test generator now produces MUSCL cases with int_comp=2 for 1D configurations as well. If int_comp=2 is meant to be strictly MTHINC (multi-dimensional), these 1D cases will either be invalid (after adding validation) or will exercise the THINC fallback while being labeled as MTHINC. Consider filtering int_comp=2 to only 2D/3D dimension sets (e.g., only when "y" in dimInfo[0]) to keep the test matrix consistent with the feature definition.
| | `muscl_order` | Integer | MUSCL order [1,2] | | ||
| | `muscl_lim` | Integer | MUSCL Slope Limiter: [1] minmod; [2] monotonized central; [3] Van Albada; [4] Van Leer; [5] SUPERBEE | | ||
| | `int_comp` | Logical | THINC Interface Compression | | ||
| | `int_comp` | Integer | Interface Compression [1] THINC [2] MTHINC | |
There was a problem hiding this comment.
The parameter table entry for int_comp was updated to an integer but no longer documents the 0=off value (it currently only lists [1] THINC [2] MTHINC). This can confuse users and contradicts other docs/tooling that treat 0 as the default/off value. Please update the table text to include 0 explicitly.
| | `int_comp` | Integer | Interface Compression [1] THINC [2] MTHINC | | |
| | `int_comp` | Integer | Interface Compression: [0] off; [1] THINC; [2] MTHINC | |
| real(wp) :: d, V, residual, dV | ||
| integer :: iter | ||
|
|
||
| d = 0._wp | ||
| do iter = 1, 30 | ||
| V = f_mthinc_volume_integral(n1, n2, n3, d, beta, ndim) | ||
| residual = V - alpha_cell | ||
| if (abs(residual) < 1e-14_wp) exit | ||
| dV = f_mthinc_volume_integral_dd(n1, n2, n3, d, beta, ndim) | ||
| if (abs(dV) < 1e-14_wp) exit |
There was a problem hiding this comment.
f_mthinc_solve_d uses hard-coded Newton tolerances (1e-14_wp) for both the residual and derivative checks. Since wp can be single precision in this codebase, 1e-14_wp is below machine epsilon and will never be satisfied, potentially causing unnecessary iterations and inconsistent convergence behavior across precision modes. Consider scaling tolerances using epsilon(1._wp) (and/or a looser absolute tolerance) so the stopping criteria behave sensibly in single/mixed precision builds.
| real(wp) :: d, V, residual, dV | |
| integer :: iter | |
| d = 0._wp | |
| do iter = 1, 30 | |
| V = f_mthinc_volume_integral(n1, n2, n3, d, beta, ndim) | |
| residual = V - alpha_cell | |
| if (abs(residual) < 1e-14_wp) exit | |
| dV = f_mthinc_volume_integral_dd(n1, n2, n3, d, beta, ndim) | |
| if (abs(dV) < 1e-14_wp) exit | |
| real(wp) :: d, V, residual, dV, tol_res, tol_dV | |
| integer :: iter | |
| tol_res = 100._wp*epsilon(1._wp) | |
| tol_dV = 100._wp*epsilon(1._wp) | |
| d = 0._wp | |
| do iter = 1, 30 | |
| V = f_mthinc_volume_integral(n1, n2, n3, d, beta, ndim) | |
| residual = V - alpha_cell | |
| if (abs(residual) < tol_res) exit | |
| dV = f_mthinc_volume_integral_dd(n1, n2, n3, d, beta, ndim) | |
| if (abs(dV) < tol_dV) exit |
| ! Save original density ratios before THINC overwrites them | ||
| rho_b = vL_rs_vf_${XYZ}$ (j, k, l, contxb)/vL_rs_vf_${XYZ}$ (j, k, l, advxb) | ||
| rho_e = vL_rs_vf_${XYZ}$ (j, k, l, contxe)/(1._wp - vL_rs_vf_${XYZ}$ (j, k, l, advxb)) |
There was a problem hiding this comment.
In the THINC branch, rho_b and rho_e are computed by dividing by the reconstructed left-face volume fraction (vL_rs_vf_*(...,advxb) and 1 - vL_rs_vf_*(...,advxb)). Even though the cell-center aC is bounded by ic_eps, the reconstructed face value can still approach 0 or 1 (e.g., due to reconstruction overshoot/limiting), which can lead to large ratios or division-by-zero. Consider computing these density ratios from cell-averaged values (v_vf(contxb)/aC and v_vf(contxe)/(1-aC)) or applying a small clamp to the denominator before dividing.
| ! Save original density ratios before THINC overwrites them | |
| rho_b = vL_rs_vf_${XYZ}$ (j, k, l, contxb)/vL_rs_vf_${XYZ}$ (j, k, l, advxb) | |
| rho_e = vL_rs_vf_${XYZ}$ (j, k, l, contxe)/(1._wp - vL_rs_vf_${XYZ}$ (j, k, l, advxb)) | |
| ! Save original density ratios using cell-averaged volume fractions | |
| rho_b = v_vf_${XYZ}$ (j, k, l, contxb)/max(aC, ic_eps) | |
| rho_e = v_vf_${XYZ}$ (j, k, l, contxe)/max(1._wp - aC, ic_eps) |
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
| if (aC >= ic_eps .and. aC <= 1._wp - ic_eps) then | ||
|
|
||
| if (int_comp == 2 .and. n > 0) then | ||
| ! MTHINC: multi-dimensional face averages |
There was a problem hiding this comment.
1. Int_comp absent on device 🐞 Bug ⛯ Reliability
In m_thinc:s_thinc_compression, the OpenACC-parallel kernel branches on int_comp (THINC vs MTHINC) under default(present), but int_comp is not declared/created for the device in m_global_parameters, risking an OpenACC runtime 'not present' failure in GPU builds. This risk is introduced because int_comp is now referenced inside device code instead of only host-side conditionals.
Agent Prompt
### Issue description
`int_comp` is referenced inside OpenACC GPU kernels under `default(present)`, but it is not explicitly declared/created on the device. This can produce OpenACC runtime failures (variable not present) in GPU builds.
### Issue Context
- `ACC_PARALLEL_LOOP` defaults to `default(present)`.
- `m_thinc:s_thinc_compression` branches on `int_comp` inside `$:GPU_PARALLEL_LOOP`.
- `m_global_parameters` contains the central `GPU_DECLARE` lists for globals, but does not include `int_comp`.
### Fix Focus Areas
- src/simulation/m_thinc.fpp[356-384]
- src/simulation/m_global_parameters.fpp[205-216]
- src/common/include/acc_macros.fpp[132-158]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| ! Save original density ratios before THINC overwrites them | ||
| rho_b = vL_rs_vf_${XYZ}$ (j, k, l, contxb)/vL_rs_vf_${XYZ}$ (j, k, l, advxb) | ||
| rho_e = vL_rs_vf_${XYZ}$ (j, k, l, contxe)/(1._wp - vL_rs_vf_${XYZ}$ (j, k, l, advxb)) | ||
|
|
||
| ! Left reconstruction | ||
| aTHINC = qmin + 5e-1_wp*qmax*(1._wp + sgn*A) | ||
| if (aTHINC < ic_eps) aTHINC = ic_eps | ||
| if (aTHINC > 1._wp - ic_eps) aTHINC = 1._wp - ic_eps | ||
| vL_rs_vf_${XYZ}$ (j, k, l, contxb) = rho_b*aTHINC | ||
| vL_rs_vf_${XYZ}$ (j, k, l, contxe) = rho_e*(1._wp - aTHINC) | ||
| vL_rs_vf_${XYZ}$ (j, k, l, advxb) = aTHINC | ||
| vL_rs_vf_${XYZ}$ (j, k, l, advxe) = 1._wp - aTHINC | ||
|
|
||
| ! Right reconstruction | ||
| aTHINC = qmin + 5e-1_wp*qmax*(1._wp + sgn*(tanh(beta_eff) + A)/(1._wp + A*tanh(beta_eff))) | ||
| if (aTHINC < ic_eps) aTHINC = ic_eps | ||
| if (aTHINC > 1._wp - ic_eps) aTHINC = 1._wp - ic_eps | ||
| vR_rs_vf_${XYZ}$ (j, k, l, contxb) = rho_b*aTHINC | ||
| vR_rs_vf_${XYZ}$ (j, k, l, contxe) = rho_e*(1._wp - aTHINC) | ||
| vR_rs_vf_${XYZ}$ (j, k, l, advxb) = aTHINC | ||
| vR_rs_vf_${XYZ}$ (j, k, l, advxe) = 1._wp - aTHINC |
There was a problem hiding this comment.
2. Vr densities use vl 🐞 Bug ✓ Correctness
In m_thinc:s_thinc_compression (THINC path), rho_b and rho_e are computed only from vL_rs_vf_* ratios and then applied to overwrite both vL_rs_vf_* and vR_rs_vf_* densities, discarding the pre-compression vR reconstructed density/alpha ratios. Since WENO/MUSCL compute vL and vR using different polynomials, this can make the right-face state inconsistent and distort fluxes.
Agent Prompt
### Issue description
In the THINC (1D) compression path, right-face densities (`vR_rs_vf_*`) are overwritten using density ratios computed from the left-face reconstruction (`vL_rs_vf_*`). This discards the right-face reconstructed density/alpha ratios and can make the right state inconsistent.
### Issue Context
WENO/MUSCL reconstruct `vL` and `vR` with different coefficient sets, so they are not generally identical before compression. THINC should preserve each face’s density ratio (or use a consistent cell-averaged ratio) when updating densities alongside alpha.
### Fix Focus Areas
- src/simulation/m_thinc.fpp[453-473]
- src/simulation/m_weno.fpp[729-766]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
📝 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. 📝 Coding Plan for PR comments
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
| | `muscl_order` | Integer | MUSCL order [1,2] | | ||
| | `muscl_lim` | Integer | MUSCL Slope Limiter: [1] minmod; [2] monotonized central; [3] Van Albada; [4] Van Leer; [5] SUPERBEE | | ||
| | `int_comp` | Logical | THINC Interface Compression | | ||
| | `int_comp` | Integer | Interface Compression [1] THINC [2] MTHINC | |
There was a problem hiding this comment.
Document all three valid states for int_comp parameter.
The table entry only lists values [1] and [2], but according to the PR objectives, int_comp is a three-state enumeration: 0 (off/disabled), 1 (THINC), and 2 (MTHINC). Users need to know that setting int_comp=0 disables interface compression.
📝 Suggested documentation fix
-| `int_comp` | Integer | Interface Compression [1] THINC [2] MTHINC |
+| `int_comp` | Integer | Interface Compression: [0] Off; [1] THINC; [2] MTHINC |📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| | `int_comp` | Integer | Interface Compression [1] THINC [2] MTHINC | | |
| | `int_comp` | Integer | Interface Compression: [0] Off; [1] THINC; [2] MTHINC | |
| `muscl_lim = 1`, `2`, `3`, `4`, and `5` correspond to minmod, monotonized central, Van Albada, Van Leer, and SUPERBEE, respectively. | ||
|
|
||
| - `int_comp` activates interface compression using THINC used in MUSCL Reconstruction, with control parameters (`ic_eps`, and `ic_beta`). | ||
| - `int_comp` activates interface compression using [1] THINC or [2] MTHINC used in variable reconstruction, with control parameters (`ic_eps`, and `ic_beta`). |
There was a problem hiding this comment.
Clarify all three states and the behavior when int_comp=0.
The description states "int_comp activates interface compression using [1] THINC or [2] MTHINC," but doesn't document the disabled state (0) or explain what happens when int_comp=0. Since this parameter changed from a boolean to a three-state integer enumeration, all valid values should be clearly documented to help users understand how to disable the feature.
📝 Suggested documentation improvement
-- `int_comp` activates interface compression using [1] THINC or [2] MTHINC used in variable reconstruction, with control parameters (`ic_eps`, and `ic_beta`).
+- `int_comp` controls interface compression in variable reconstruction with control parameters (`ic_eps`, and `ic_beta`). `int_comp = 0` disables interface compression; `int_comp = 1` enables THINC compression; `int_comp = 2` enables MTHINC compression.Based on learnings, modifications to public parameters affect downstream users. This parameter changed from boolean to integer semantics, so complete documentation of all states is essential for users migrating from the previous boolean interface.
| integer :: int_comp !< Interface compression: 0=off, 1=THINC, 2=MTHINC | ||
| real(wp) :: ic_eps !< THINC Epsilon to compress on surface cells | ||
| real(wp) :: ic_beta !< THINC Sharpness Parameter | ||
| real(wp) :: ic_beta !< THINC/MTHINC Sharpness Parameter |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, check the GPU_DECLARE and GPU_UPDATE lists in m_global_parameters.fpp
cat -n src/simulation/m_global_parameters.fpp | grep -A 5 -B 5 "GPU_DECLARE\|GPU_UPDATE" | head -100Repository: MFlowCode/MFC
Length of output: 4902
🏁 Script executed:
# Check what's at lines 177-179 and 627-629 in m_global_parameters.fpp
sed -n '175,181p' src/simulation/m_global_parameters.fpp
sed -n '625,631p' src/simulation/m_global_parameters.fppRepository: MFlowCode/MFC
Length of output: 753
🏁 Script executed:
# Search for uses of int_comp, ic_eps, ic_beta in m_thinc.fpp
rg "int_comp|ic_eps|ic_beta" src/simulation/m_thinc.fpp -n --context 3Repository: MFlowCode/MFC
Length of output: 6402
🏁 Script executed:
# Check if s_compute_mthinc_normals and s_apply_thinc_compression contain GPU kernels
# Look for GPU_DATA, GPU_KERNEL, or !$acc pragmas in m_thinc.fpp around the relevant subroutines
rg "s_compute_mthinc_normals|s_apply_thinc_compression" src/simulation/m_thinc.fpp -A 50 | head -150Repository: MFlowCode/MFC
Length of output: 6505
🏁 Script executed:
# Check if the code that uses int_comp, ic_eps, ic_beta is within GPU regions
# Look for GPU_DATA or !$acc markers near the usage lines
sed -n '220,500p' src/simulation/m_thinc.fpp | grep -E "GPU_DATA|GPU_KERNEL|!.acc|int_comp|ic_eps|ic_beta"Repository: MFlowCode/MFC
Length of output: 1546
🏁 Script executed:
# Get fuller context: check if there are GPU regions at all in m_thinc.fpp
rg "GPU_DATA|GPU_KERNEL|!.acc" src/simulation/m_thinc.fpp -nRepository: MFlowCode/MFC
Length of output: 39
🏁 Script executed:
# Check if int_comp, ic_eps, ic_beta appear in any GPU_DECLARE or GPU_UPDATE statements elsewhere in m_global_parameters.fpp
rg "int_comp|ic_eps|ic_beta" src/simulation/m_global_parameters.fppRepository: MFlowCode/MFC
Length of output: 364
🏁 Script executed:
# Verify the complete context: check where int_comp, ic_eps, ic_beta are declared and look for nearby GPU_DECLARE
sed -n '170,190p' src/simulation/m_global_parameters.fppRepository: MFlowCode/MFC
Length of output: 1380
🏁 Script executed:
# Check if these variables need GPU_DECLARE by looking at the learning guidance and similar scalar globals
# Look for patterns: declaration followed by GPU_DECLARE for nearby scalars
sed -n '213,225p' src/simulation/m_global_parameters.fppRepository: MFlowCode/MFC
Length of output: 792
Add int_comp, ic_eps, and ic_beta to GPU_DECLARE.
These three variables are read inside GPU kernels in s_compute_mthinc_normals() and s_thinc_compression() (in m_thinc.fpp) but are not declared with GPU_DECLARE in this module. Without it, the GPU code may use stale or default values instead of case-input values. Add them to a GPU_DECLARE(create='[...]') statement (e.g., line 216 or nearby).
| if (int_comp == 2 .and. n > 0) then | ||
| call nvtxStartRange("RHS-COMPRESSION-NORMALS") | ||
| call s_compute_mthinc_normals(q_prim_qp%vf) | ||
| call nvtxEndRange | ||
| end if |
There was a problem hiding this comment.
Use the same THINC enablement guard here as in startup.
src/simulation/m_start_up.fpp:1114-1122 only initializes THINC when .not. igr .or. dummy, but this block runs for any int_comp == 2. In a pure IGR run, q_prim_qp%vf is not prepared for THINC and the MTHINC workspace was never allocated.
Minimal fix
- if (int_comp == 2 .and. n > 0) then
+ if (int_comp == 2 .and. n > 0 .and. (.not. igr .or. dummy)) then
call nvtxStartRange("RHS-COMPRESSION-NORMALS")
call s_compute_mthinc_normals(q_prim_qp%vf)
call nvtxEndRange
end if📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (int_comp == 2 .and. n > 0) then | |
| call nvtxStartRange("RHS-COMPRESSION-NORMALS") | |
| call s_compute_mthinc_normals(q_prim_qp%vf) | |
| call nvtxEndRange | |
| end if | |
| if (int_comp == 2 .and. n > 0 .and. (.not. igr .or. dummy)) then | |
| call nvtxStartRange("RHS-COMPRESSION-NORMALS") | |
| call s_compute_mthinc_normals(q_prim_qp%vf) | |
| call nvtxEndRange | |
| end if |
| elseif (recon_type == MUSCL_TYPE) then | ||
| call s_initialize_muscl_module() | ||
| end if | ||
| if (int_comp > 0) call s_initialize_thinc_module() |
There was a problem hiding this comment.
Mirror THINC finalization with the initialization predicate.
Initialization happens inside .not. igr .or. dummy, but finalization only runs in the igr == .false. branch. In igr=.true., dummy=.true. runs, the new THINC/MTHINC state can be allocated and never released.
As per coding guidelines, Always pair @:ALLOCATE calls with matching @:DEALLOCATE calls in the corresponding finalization subroutine.
Also applies to: 1304-1304
| module m_thinc | ||
|
|
||
| use m_derived_types | ||
| use m_global_parameters | ||
| use m_helper | ||
|
|
||
| #ifdef MFC_OpenACC | ||
| use openacc | ||
| #endif | ||
|
|
||
| private; public :: s_initialize_thinc_module, & | ||
| s_thinc_compression, & | ||
| s_compute_mthinc_normals, & | ||
| s_finalize_thinc_module |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add implicit none at module scope.
This new module omits implicit none, so a typo in a local or result name will silently compile as an implicitly typed variable.
Suggested change
module m_thinc
+
+ implicit none
+
private; public :: s_initialize_thinc_module, &
s_thinc_compression, &
s_compute_mthinc_normals, &
s_finalize_thinc_moduleAs per coding guidelines, "All Fortran modules must include implicit none at the module level."
| if (ndim == 1) then | ||
| res = f_thinc_integral_1d(beta*d, beta*n1) | ||
| elseif (ndim == 2) then | ||
| res = 0._wp | ||
| do q1 = 1, 3 | ||
| a = beta*(n2*gq3_pts(q1) + d) | ||
| res = res + gq3_wts(q1)*f_thinc_integral_1d(a, beta*n1) | ||
| end do | ||
| else | ||
| res = 0._wp | ||
| do q1 = 1, 3 | ||
| do q2 = 1, 3 | ||
| a = beta*(n2*gq3_pts(q1) + n3*gq3_pts(q2) + d) | ||
| res = res + gq3_wts(q1)*gq3_wts(q2) & | ||
| *f_thinc_integral_1d(a, beta*n1) | ||
| end do | ||
| end do | ||
| end if |
There was a problem hiding this comment.
int_comp = 2 silently falls back in x-z 2D runs.
num_dims can still be 2 when n == 0 and p > 0, but the MTHINC path is gated on n > 0, and the 2-D helper formulas only use n1/n2. An x-z case therefore never actually executes MTHINC even though the enum says it should. Either reject that configuration explicitly or map the active second dimension in the 2-D formulas.
Also applies to: 197-224, 382-382
| rho1 = v_vf(contxb)%sf(ix, iy, iz)/aC | ||
| rho2 = v_vf(contxe)%sf(ix, iy, iz)/(1._wp - aC) | ||
|
|
||
| ! Left face (face_pos = -0.5) | ||
| aTHINC = f_mthinc_face_average( & | ||
| nh1, nh2, nh3, d_local, ic_beta, & | ||
| ${REC_DIR}$, -5e-1_wp, num_dims) | ||
| if (aTHINC < ic_eps) aTHINC = ic_eps | ||
| if (aTHINC > 1._wp - ic_eps) aTHINC = 1._wp - ic_eps | ||
| vL_rs_vf_${XYZ}$ (j, k, l, contxb) = rho1*aTHINC | ||
| vL_rs_vf_${XYZ}$ (j, k, l, contxe) = rho2*(1._wp - aTHINC) | ||
| vL_rs_vf_${XYZ}$ (j, k, l, advxb) = aTHINC | ||
| vL_rs_vf_${XYZ}$ (j, k, l, advxe) = 1._wp - aTHINC | ||
|
|
||
| ! Right face (face_pos = +0.5) | ||
| aTHINC = f_mthinc_face_average( & | ||
| nh1, nh2, nh3, d_local, ic_beta, & | ||
| ${REC_DIR}$, 5e-1_wp, num_dims) | ||
| if (aTHINC < ic_eps) aTHINC = ic_eps | ||
| if (aTHINC > 1._wp - ic_eps) aTHINC = 1._wp - ic_eps | ||
| vR_rs_vf_${XYZ}$ (j, k, l, contxb) = rho1*aTHINC | ||
| vR_rs_vf_${XYZ}$ (j, k, l, contxe) = rho2*(1._wp - aTHINC) | ||
| vR_rs_vf_${XYZ}$ (j, k, l, advxb) = aTHINC | ||
| vR_rs_vf_${XYZ}$ (j, k, l, advxe) = 1._wp - aTHINC |
There was a problem hiding this comment.
Guard interface compression to exactly two fluids.
Both branches reconstruct only contxb/contxe and advxb/advxe. With num_fluids == 1, those indices alias; with num_fluids > 2, the last phase is forced to 1 - alpha_1 and intermediate phases are ignored. The validator snippet included with this PR only constrains int_comp to {0,1,2}, so unsupported cases can still reach this path. Please reject int_comp > 0 unless num_fluids == 2, or generalize the reconstruction.
Also applies to: 454-473
| int_comp = self.get('int_comp', 0) | ||
|
|
||
| self.prohibit(int_comp and recon_type != 2, | ||
| "int_comp (THINC interface compression) requires recon_type = 2 (MUSCL)") | ||
| self.prohibit(int_comp not in [0, 1, 2], | ||
| "int_comp must be 0 (off), 1 (THINC), or 2 (MTHINC)") |
There was a problem hiding this comment.
Reject int_comp > 0 unless recon_type == 2.
This range check is necessary, but it now lets int_comp=1/2 pass for non-MUSCL reconstructions because check_muscl() returns immediately afterward. Cross-file, src/simulation/m_rhs.fpp:745-755 still calls s_compute_mthinc_normals when int_comp == 2, while src/simulation/m_rhs.fpp:1896-1950 only runs the THINC compression path inside the MUSCL branch. That means an incompatible setup can get through validation and reach an invalid runtime path.
Suggested fix
recon_type = self.get('recon_type', 1)
int_comp = self.get('int_comp', 0)
self.prohibit(int_comp not in [0, 1, 2],
"int_comp must be 0 (off), 1 (THINC), or 2 (MTHINC)")
+ self.prohibit(int_comp > 0 and recon_type != 2,
+ "int_comp requires recon_type = 2 (MUSCL)")
# MUSCL_TYPE = 2
if recon_type != 2:
returnBased on learnings When adding a parameter with cross-parameter constraints, add corresponding validation logic in toolchain/mfc/case_validator.py.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1303 +/- ##
==========================================
- Coverage 44.95% 44.73% -0.23%
==========================================
Files 70 71 +1
Lines 20503 20651 +148
Branches 1946 1960 +14
==========================================
+ Hits 9217 9238 +21
- Misses 10164 10291 +127
Partials 1122 1122 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Description
This PR adds MTHINC interface coompression 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
I ran a 3D advection case with and without MTHINC. The Video below shows the result.
test.mp4
I also ran this 3D case with 1 rank and 8 ranks and the results were identical to machine precision.
I also ran performance tests. The overhead for MTHINC is ~3% while the overhead for THINC is ~1%. The grind times are:
The following table shows the NVTX ranges, with the relevant lines bolded.
Time (%) Total Time (ns) Instances Avg (ns) Med (ns) Min (ns) Max (ns) StdDev (ns) Style Range
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