Skip to content

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.

    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
    @sbryngelson sbryngelson marked this pull request as draft July 19, 2025 21:39
    @sbryngelson sbryngelson self-requested a review August 19, 2025 19:01
    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