Refactor m_riemann_solvers Module (HLL Solver)

[Malmahrouqi3]

User description

Description

Reduced PR from (#855),

Essentially, I refactored math-critical items in s_hll_riemann_solver subroutine.

---

PR Type

Enhancement

---

Description

• Refactored HLL Riemann solver for improved code efficiency

• Consolidated repetitive loops into vectorized operations

• Simplified hypoelasticity energy flux calculations

• Optimized magnetic field flux computations

---

Changes diagram

flowchart LR
  A["Original HLL Solver"] --> B["Loop Consolidation"]
  B --> C["Vector Operations"]
  C --> D["Simplified Calculations"]
  D --> E["Optimized HLL Solver"]

Loading

---

Changes walkthrough 📝

Relevant files

Enhancement

m_riemann_solvers.fpp HLL solver optimization and code consolidation                      src/simulation/m_riemann_solvers.fpp Consolidated velocity and Reynolds number loops for better efficiency Replaced explicit component assignments with vectorized array operations Simplified hypoelasticity energy flux from dimension-specific cases to unified loop Optimized magnetic field flux calculations using loop structures +62/-159

---

Need help?

Type /help how to ... in the comments thread for any questions about Qodo Merge usage.

Check out the documentation for more information.

[qodo-merge-pro]

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪

🧪 No relevant tests

🔒 No security concerns identified

⚡ Recommended focus areas for review Missing Tests This PR refactors critical mathematical computations in the HLL Riemann solver but does not include any tests to verify the correctness of the vectorized operations and consolidated loops. Given the complexity of fluid dynamics calculations, comprehensive tests are essential to ensure the refactored code produces identical results. vel_L(i) = qL_prim_rs${XYZ}$_vf(j, k, l, contxe + i) vel_R(i) = qR_prim_rs${XYZ}$_vf(j + 1, k, l, contxe + i) vel_L_rms = vel_L_rms + vel_L(i)**2._wp vel_R_rms = vel_R_rms + vel_R(i)**2._wp end do Loop Bounds The consolidated loops use hardcoded bounds that may not be consistent across different physics configurations. The magnetic field loop uses do i = 0, 2 and do i = 0, 1 which assumes 3D components, but the bounds should be validated against the actual number of dimensions and components available in different simulation modes. do i = 0, 1 flux_rsx_vf(j, k, l, B_idx%beg + i) = (s_M*(vel_R(1)*B%R(2 + i) - vel_R(2 + i)*Bx0) & - s_P*(vel_L(1)*B%L(2 + i) - vel_L(2 + i)*Bx0) & + s_M*s_P*(B%L(2 + i) - B%R(2 + i)))/(s_M - s_P) end do else ! 2D/3D: Bx, By, Bz /= const. but zero flux component in the same direction ! B_x d/d${XYZ}$ flux = (1 - delta(x,${XYZ}$)) * (v_${XYZ}$ * B_x - v_x * B_${XYZ}$) ! B_y d/d${XYZ}$ flux = (1 - delta(y,${XYZ}$)) * (v_${XYZ}$ * B_y - v_y * B_${XYZ}$) ! B_z d/d${XYZ}$ flux = (1 - delta(z,${XYZ}$)) * (v_${XYZ}$ * B_z - v_z * B_${XYZ}$) !$acc loop seq do i = 0, 2 flux_rs${XYZ}$_vf(j, k, l, B_idx%beg + i) = (1 - dir_flg(i + 1))*( & s_M*(vel_R(dir_idx(1))*B%R(i + 1) - vel_R(i + 1)*B%R(norm_dir)) - & s_P*(vel_L(dir_idx(1))*B%L(i + 1) - vel_L(i + 1)*B%L(norm_dir)) + & s_M*s_P*(B%L(i + 1) - B%R(i + 1)))/(s_M - s_P) end do Variable Scope The new variables flux_tau_L and flux_tau_R are declared at the subroutine level but only used in the hypoelasticity section. This could lead to uninitialized variable usage in other code paths or unnecessary memory allocation. These variables should be declared locally within the hypoelasticity conditional block. real(wp) :: flux_tau_L, flux_tau_R

[Copilot]

Pull Request Overview

This PR refactors the HLL Riemann solver logic within the m_riemann_solvers module to improve math-critical routines and streamline flux computations. Key changes include the addition of new variables for tau fluxes, consolidation and vectorization of loops for performance improvements (including OpenACC directives), and rewriting flux computations using array slicing and loops.

Comments suppressed due to low confidence (4)

src/simulation/m_riemann_solvers.fpp:464

• Consider adding inline comments explaining the rationale for combining Re_L and Re_R computations within a single loop, which will aid future maintainers in understanding the symmetry and performance benefits of this refactor.

                                    Re_R(i) = dflt_real

src/simulation/m_riemann_solvers.fpp:536

• [nitpick] Confirm that the vectorized array-slice operations replicate the intended element‐by‐element computation accurately, and consider adding a brief comment to clarify this design choice for future readers.

                                b4%L(1:3) = B%L(1:3)/Ga%L + Ga%L*vel_L(1:3)*vdotB%L

src/simulation/m_riemann_solvers.fpp:962

• Verify that looping from 0 to 2 and accessing array elements as (i + 1) is consistent with the rest of the module’s 1-indexed conventions to avoid any potential off-by-one indexing errors.

                                    do i = 0, 2

src/simulation/m_riemann_solvers.fpp:752

• Ensure that the newly introduced !$acc loop seq directives in the flux computations are benchmarked to confirm that they provide the expected GPU performance benefits without introducing overhead.

                            if (mhd .and. (.not. relativity)) then

[qodo-merge-pro]

PR Code Suggestions ✨

No code suggestions found for the PR.

[Malmahrouqi3]

Loop Bounds
The consolidated loops use hardcoded bounds that may not be consistent across different physics configurations. The magnetic field loop uses do i = 0, 2 and do i = 0, 1 which assumes 3D components, but the bounds should be validated against the actual number of dimensions and components available in different simulation modes.

Variable Scope
The new variables flux_tau_L and flux_tau_R are declared at the subroutine level but only used in the hypoelasticity section. This could lead to uninitialized variable usage in other code paths or unnecessary memory allocation. These variables should be declared locally within the hypoelasticity conditional block.

For the loop bounds, I basically combined identical flux terms with incrementally different indexing, so just making a do loop instead of hardcoding calcs for each flux term.

[Malmahrouqi3]

Pull Request Overview

This PR refactors the HLL Riemann solver logic within the m_riemann_solvers module to improve math-critical routines and streamline flux computations. Key changes include the addition of new variables for tau fluxes, consolidation and vectorization of loops for performance improvements (including OpenACC directives), and rewriting flux computations using array slicing and loops.

Comments suppressed due to low confidence (4)
src/simulation/m_riemann_solvers.fpp:464

• Consider adding inline comments explaining the rationale for combining Re_L and Re_R computations within a single loop, which will aid future maintainers in understanding the symmetry and performance benefits of this refactor.

                                    Re_R(i) = dflt_real

src/simulation/m_riemann_solvers.fpp:536

• [nitpick] Confirm that the vectorized array-slice operations replicate the intended element‐by‐element computation accurately, and consider adding a brief comment to clarify this design choice for future readers.

                                b4%L(1:3) = B%L(1:3)/Ga%L + Ga%L*vel_L(1:3)*vdotB%L

src/simulation/m_riemann_solvers.fpp:962

• Verify that looping from 0 to 2 and accessing array elements as (i + 1) is consistent with the rest of the module’s 1-indexed conventions to avoid any potential off-by-one indexing errors.

                                    do i = 0, 2

src/simulation/m_riemann_solvers.fpp:752

• Ensure that the newly introduced !$acc loop seq directives in the flux computations are benchmarked to confirm that they provide the expected GPU performance benefits without introducing overhead.

                            if (mhd .and. (.not. relativity)) then

1. Pre-existed thus not much to do about it.
2. Three terms with their corresponding indices combined into one.
3. It not about starting at 0 or 2, as they do not represent actually a standalone index at all.
   Context

flux_rs${XYZ}$_vf(j, k, l, B_idx%beg + i) = (1 - dir_flg(i + 1))*( &
                                        s_M*(vel_R(dir_idx(1))*B%R(i + 1) - vel_R(i + 1)*B%R(norm_dir)) - &
                                        s_P*(vel_L(dir_idx(1))*B%L(i + 1) - vel_L(i + 1)*B%L(norm_dir)) + &
                                        s_M*s_P*(B%L(i + 1) - B%R(i + 1)))/(s_M - s_P)

4. Should be handled during merge.

[Malmahrouqi3]

Pushing for the GitHub CI testing to catch any errors after verifying the pass of HLL specific tests locally.

[codecov]

Codecov Report

Attention: Patch coverage is 48.27586% with 15 lines in your changes missing coverage. Please review.

Project coverage is 46.01%. Comparing base (4864d36) to head (4245a69).
Report is 2 commits behind head on master.

Files with missing lines	Patch %	Lines
src/simulation/m_riemann_solvers.fpp	48.27%	15 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #910      +/-   ##
==========================================
+ Coverage   45.98%   46.01%   +0.03%     
==========================================
  Files          68       68              
  Lines       18629    18591      -38     
  Branches     2239     2238       -1     
==========================================
- Hits         8566     8555      -11     
+ Misses       8711     8685      -26     
+ Partials     1352     1351       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.