Skip to content

Refactor m_riemann_solvers Module (HLLC Solver Subroutine) #912

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

Malmahrouqi3
Copy link
Collaborator

@Malmahrouqi3 Malmahrouqi3 commented Jul 1, 2025

User description

Description

Reduced PR from (#855),

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


PR Type

Enhancement


Description

  • Consolidate variable initialization at loop start

  • Merge duplicate loops for left/right state calculations

  • Combine hypoelastic and hyperelastic energy adjustments

  • Optimize Reynolds number computation loops


Changes diagram

flowchart LR
  A["Variable Initialization"] --> B["Loop Consolidation"]
  B --> C["Energy Adjustments Merge"]
  C --> D["Reynolds Number Optimization"]
  D --> E["Improved Code Structure"]
Loading

Changes walkthrough 📝

Relevant files
Enhancement
m_riemann_solvers.fpp
HLLC solver code structure optimization                                   

src/simulation/m_riemann_solvers.fpp

  • Consolidate variable initialization at beginning of loops
  • Merge separate left/right state calculation loops into single loops
  • Combine hypoelastic and hyperelastic energy adjustment logic
  • Optimize Reynolds number calculations by merging loops
  • +145/-264

    Need help?
  • Type /help how to ... in the comments thread for any questions about Qodo Merge usage.
  • Check out the documentation for more information.
  • @Copilot Copilot AI review requested due to automatic review settings July 1, 2025 03:28
    @Malmahrouqi3 Malmahrouqi3 requested a review from a team as a code owner July 1, 2025 03:28
    Copy link

    qodo-merge-pro bot commented Jul 1, 2025

    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

    Logic Error

    In the merged loop for mpp_lim case, the left state normalization is now inside the same loop as the accumulation, which could cause incorrect calculations since alpha_L_sum is being modified while still being accumulated.

        do i = 1, num_fluids
            qL_prim_rs${XYZ}$_vf(j, k, l, i) = max(0._wp, qL_prim_rs${XYZ}$_vf(j, k, l, i))
            qL_prim_rs${XYZ}$_vf(j, k, l, E_idx + i) = min(max(0._wp, qL_prim_rs${XYZ}$_vf(j, k, l, E_idx + i)), 1._wp)
            alpha_L_sum = alpha_L_sum + qL_prim_rs${XYZ}$_vf(j, k, l, E_idx + i)
            qR_prim_rs${XYZ}$_vf(j + 1, k, l, i) = max(0._wp, qR_prim_rs${XYZ}$_vf(j + 1, k, l, i))
            qR_prim_rs${XYZ}$_vf(j + 1, k, l, E_idx + i) = min(max(0._wp, qR_prim_rs${XYZ}$_vf(j + 1, k, l, E_idx + i)), 1._wp)
            alpha_R_sum = alpha_R_sum + qR_prim_rs${XYZ}$_vf(j + 1, k, l, E_idx + i)
        end do
    
        !$acc loop seq
        do i = 1, num_fluids
            qL_prim_rs${XYZ}$_vf(j, k, l, E_idx + i) = qL_prim_rs${XYZ}$_vf(j, k, l, E_idx + i)/max(alpha_L_sum, sgm_eps)
            qR_prim_rs${XYZ}$_vf(j + 1, k, l, E_idx + i) = qR_prim_rs${XYZ}$_vf(j + 1, k, l, E_idx + i)/max(alpha_R_sum, sgm_eps)
        end do
    end if
    Duplicate Calculation

    The shear modulus G_L and G_R are calculated twice in the hyperelasticity branch - once at the beginning of the energy adjustment block and again specifically for hyperelasticity, leading to redundant computation.

    G_L = 0._wp; G_R = 0._wp;
    !$acc loop seq
    do i = 1, num_fluids
        ! Mixture left and right shear modulus
        G_L = G_L + alpha_L(i)*Gs(i)
        G_R = G_R + alpha_R(i)*Gs(i)
    end do
    Missing Assignment

    The missing assignment for Re_L(i) calculation completion in the viscous Reynolds number computation could lead to incorrect viscous flux calculations.

    Re_L(i) = 1._wp/max(Re_L(i), sgm_eps)
    Re_R(i) = 1._wp/max(Re_R(i), sgm_eps)

    Copy link
    Contributor

    @Copilot Copilot AI left a comment

    Choose a reason for hiding this comment

    The reason will be displayed to describe this comment to others. Learn more.

    Pull Request Overview

    This PR refactors the s_hllc_riemann_solver subroutine in m_riemann_solvers.fpp to streamline variable initialization, consolidate energy adjustment logic, and introduce a loop_end helper for fluid loops.

    • Consolidated zero-initialization of primitive-state variables at the start of each cell loop
    • Merged hypoelastic and hyperelastic energy adjustments into a single conditional block
    • Introduced loop_end to simplify fluid‐count branching

    Copy link

    qodo-merge-pro bot commented Jul 1, 2025

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Impact
    General
    Remove duplicate shear modulus calculation

    The shear modulus calculation for G_L and G_R is duplicated in both the outer
    scope and within the hyperelasticity branch. Remove the redundant calculation
    inside the hyperelasticity branch to avoid unnecessary computation and potential
    inconsistency.

    src/simulation/m_riemann_solvers.fpp [1381-1431]

     if (hypoelasticity .or. hyperelasticity) then
         G_L = 0._wp; G_R = 0._wp
         !$acc loop seq
         do i = 1, num_fluids
             G_L = G_L + alpha_L(i)*Gs(i)
             G_R = G_R + alpha_R(i)*Gs(i)
         end do
         if (hypoelasticity) then
             ...
         else if (hyperelasticity) then
             ...
    -        G_L = 0._wp; G_R = 0._wp;
    -        !$acc loop seq
    -        do i = 1, num_fluids
    -            ! Mixture left and right shear modulus
    -            G_L = G_L + alpha_L(i)*Gs(i)
    -            G_R = G_R + alpha_R(i)*Gs(i)
    -        end do
    +        ! Elastic contribution to energy if G large enough
    +        if (G_L > verysmall .and. G_R > verysmall) then
    +            E_L = E_L + G_L*qL_prim_rs${XYZ}$_vf(j, k, l, xiend + 1)
    +            E_R = E_R + G_R*qR_prim_rs${XYZ}$_vf(j + 1, k, l, xiend + 1)
    +        end if
             ...
         end if
     end if
    Suggestion importance[1-10]: 6

    __

    Why: The suggestion correctly identifies that the calculation for G_L and G_R is duplicated, which is a code quality issue introduced in the PR.

    Low
    • Update

    @Malmahrouqi3
    Copy link
    Collaborator Author

    Pushing this after ensuring select buggy tests that failed prior have passed neatly.

    Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
    Copy link

    codecov bot commented Jul 1, 2025

    Codecov Report

    Attention: Patch coverage is 22.22222% with 70 lines in your changes missing coverage. Please review.

    Project coverage is 44.17%. Comparing base (6f58eec) to head (ec4151e).

    Files with missing lines Patch % Lines
    src/simulation/m_riemann_solvers.fpp 22.22% 66 Missing and 4 partials ⚠️
    Additional details and impacted files
    @@            Coverage Diff             @@
    ##           master     #912      +/-   ##
    ==========================================
    + Coverage   44.15%   44.17%   +0.02%     
    ==========================================
      Files          68       68              
      Lines       18347    18309      -38     
      Branches     2227     2225       -2     
    ==========================================
    - Hits         8101     8088      -13     
    + Misses       8943     8922      -21     
    + Partials     1303     1299       -4     

    ☔ 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.

    @sbryngelson sbryngelson self-requested a review July 4, 2025 01:59
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Development

    Successfully merging this pull request may close these issues.

    2 participants