Skip to content

Conversation

@hyeoksu-lee
Copy link
Contributor

@hyeoksu-lee hyeoksu-lee commented Aug 6, 2025

User description

Description

This PR fixes a bug in QBMM module. There are missing Re_inv in coeffs with indices 22 and 23. I found this bug based on dimensional analysis and @anandrdbz confirmed this by directly expanding out the equation.

Test suites failed with corrected form are also updated in this PR.

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Something else

Scope

  • This PR comprises a set of related changes with a common goal

If you cannot check the above box, please split your PR into multiple PRs that each have a common goal.

How Has This Been Tested?

  • What computers and compilers did you use to test this: MacBook M4 Pro

PR Type

Bug fix


Description

  • Fix missing Re_inv terms in QBMM coefficients

  • Update test suite golden files for corrected equations


Diagram Walkthrough

flowchart LR
  A["QBMM coefficients"] --> B["Add Re_inv to coeffs[22,23]"]
  B --> C["Update test golden files"]
Loading

File Walkthrough

Relevant files
Bug fix
1 files
m_qbmm.fpp
Add missing Re_inv terms to coefficients                                 
+4/-4     
Tests
10 files
golden-metadata.txt
Update test metadata for corrected QBMM                                   
+42/-77 
golden-metadata.txt
Update test metadata for corrected QBMM                                   
+39/-102
golden-metadata.txt
Update test metadata for corrected QBMM                                   
+38/-101
golden-metadata.txt
Update test metadata for corrected QBMM                                   
+37/-100
golden-metadata.txt
Update test metadata for corrected QBMM                                   
+36/-99 
golden-metadata.txt
Update test metadata for corrected QBMM                                   
+37/-100
golden-metadata.txt
Update test metadata for corrected QBMM                                   
+36/-99 
golden-metadata.txt
Update test metadata for corrected QBMM                                   
+78/-44 
golden-metadata.txt
Update test metadata for corrected QBMM                                   
+70/-36 
golden-metadata.txt
Update test metadata for corrected QBMM                                   
+70/-36 
Additional files
10 files
golden.txt +6/-6     
golden.txt +8/-8     
golden.txt +45/-45 
golden.txt +31/-31 
golden.txt +14/-14 
golden.txt +20/-20 
golden.txt +20/-20 
golden.txt +32/-32 
golden.txt +32/-32 
golden.txt +66/-66 

@qodo-merge-pro
Copy link
Contributor

qodo-merge-pro bot commented Aug 6, 2025

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
🧪 PR contains tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

Code Duplication

The same fix is applied to two identical code blocks at lines 611-612 and 682-683. This suggests potential code duplication that could be refactored to reduce maintenance burden.

                            coeffs(22, i1, i2) = -i2*4._wp*Re_inv/(rho*rho*c)
                            coeffs(23, i1, i2) = -i2*4._wp*Re_inv/(rho*rho*c*c)
                            coeffs(24, i1, i2) = i2*16._wp*Re_inv*Re_inv/(rho*rho*c)
                            if (.not. f_is_default(Web)) then
                                coeffs(25, i1, i2) = i2*8._wp*Re_inv/Web/(rho*rho*c)
                            end if
                            coeffs(26, i1, i2) = -12._wp*i2*gam*Re_inv/(rho*rho*c*c)
                        end if
                        coeffs(27, i1, i2) = 3._wp*i2*gam*R_v*Tw/(c*rho)
                        coeffs(28, i1, i2) = 3._wp*i2*gam*R_v*Tw/(c*c*rho)
                        if (.not. f_is_default(Re_inv)) then
                            coeffs(29, i1, i2) = 12._wp*i2*gam*R_v*Tw*Re_inv/(rho*rho*c*c)
                        end if
                        coeffs(30, i1, i2) = 3._wp*i2*gam/(c*rho)
                        coeffs(31, i1, i2) = 3._wp*i2*gam/(c*c*rho)
                        if (.not. f_is_default(Re_inv)) then
                            coeffs(32, i1, i2) = 12._wp*i2*gam*Re_inv/(rho*rho*c*c)
                        end if
                    end if
                end if
            end do; end do

    end subroutine s_coeff_nonpoly

!Coefficient array for polytropic model (pb for each R0 bin accounted for in wght_pb)
    pure subroutine s_coeff(pres, rho, c, coeffs)
        $:GPU_ROUTINE(function_name='s_coeff',parallelism='[seq]', &
            & cray_inline=True)

        real(wp), intent(in) :: pres, rho, c
        real(wp), dimension(nterms, 0:2, 0:2), intent(out) :: coeffs

        integer :: i1, i2

        coeffs = 0._wp

        do i2 = 0, 2; do i1 = 0, 2
                if ((i1 + i2) <= 2) then
                    if (bubble_model == 3) then
                        ! RPE
                        coeffs(1, i1, i2) = -1._wp*i2*pres/rho
                        coeffs(2, i1, i2) = -3._wp*i2/2._wp
                        coeffs(3, i1, i2) = i2/rho
                        coeffs(4, i1, i2) = i1
                        if (.not. f_is_default(Re_inv)) coeffs(5, i1, i2) = -4._wp*i2*Re_inv/rho
                        if (.not. f_is_default(Web)) coeffs(6, i1, i2) = -2._wp*i2/Web/rho
                        coeffs(7, i1, i2) = i2*pv/rho
                    else if (bubble_model == 2) then
                        ! KM with approximation of 1/(1-V/C) = 1+V/C
                        coeffs(1, i1, i2) = -3._wp*i2/2._wp
                        coeffs(2, i1, i2) = -i2/c
                        coeffs(3, i1, i2) = i2/(2._wp*c*c)
                        coeffs(4, i1, i2) = -i2*pres/rho
                        coeffs(5, i1, i2) = -2._wp*i2*pres/(c*rho)
                        coeffs(6, i1, i2) = -i2*pres/(c*c*rho)
                        coeffs(7, i1, i2) = i2/rho
                        coeffs(8, i1, i2) = 2._wp*i2/(c*rho)
                        coeffs(9, i1, i2) = i2/(c*c*rho)
                        coeffs(10, i1, i2) = -3._wp*i2*gam/(c*rho)
                        coeffs(11, i1, i2) = -3._wp*i2*gam/(c*c*rho)
                        coeffs(12, i1, i2) = i1
                        coeffs(13, i1, i2) = i2*(pv)/rho
                        coeffs(14, i1, i2) = 2._wp*i2*(pv)/(c*rho)
                        coeffs(15, i1, i2) = i2*(pv)/(c*c*rho)
                        if (.not. f_is_default(Re_inv)) coeffs(16, i1, i2) = -i2*4._wp*Re_inv/rho
                        if (.not. f_is_default(Web)) coeffs(17, i1, i2) = -i2*2._wp/Web/rho
                        if (.not. f_is_default(Re_inv)) then
                            coeffs(18, i1, i2) = i2*6._wp*Re_inv/(rho*c)
                            coeffs(19, i1, i2) = -i2*2._wp*Re_inv/(rho*c*c)
                            coeffs(20, i1, i2) = i2*4._wp*pres*Re_inv/(rho*rho*c)
                            coeffs(21, i1, i2) = i2*4._wp*pres*Re_inv/(rho*rho*c*c)
                            coeffs(22, i1, i2) = -i2*4._wp*Re_inv/(rho*rho*c)
                            coeffs(23, i1, i2) = -i2*4._wp*Re_inv/(rho*rho*c*c)

@qodo-merge-pro
Copy link
Contributor

qodo-merge-pro bot commented Aug 6, 2025

PR Code Suggestions ✨

No code suggestions found for the PR.

@codecov
Copy link

codecov bot commented Aug 6, 2025

Codecov Report

❌ Patch coverage is 0% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 43.23%. Comparing base (e56d6dc) to head (cfb50a0).
⚠️ Report is 1 commits behind head on master.

Files with missing lines Patch % Lines
src/simulation/m_qbmm.fpp 0.00% 4 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##           master     #975   +/-   ##
=======================================
  Coverage   43.23%   43.23%           
=======================================
  Files          70       70           
  Lines       20109    20109           
  Branches     2513     2513           
=======================================
  Hits         8695     8695           
  Misses       9877     9877           
  Partials     1537     1537           

☔ 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
Copy link
Member

good find, thanks! we should put a markdown table or something somewhere on how these are formed/where they come from

@hyeoksu-lee
Copy link
Contributor Author

@sbryngelson I totally agree. I couldn't find any documentation about its formation, so I couldn't derive it from the equations myself.

@anandrdbz
Copy link
Contributor

It comes from binomial expansion involving 2 terms, but I'll try to document it

@sbryngelson sbryngelson merged commit 8937245 into MFlowCode:master Aug 10, 2025
30 checks passed
@hyeoksu-lee hyeoksu-lee deleted the qbmm branch August 26, 2025 19:47
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.

3 participants