-
Notifications
You must be signed in to change notification settings - Fork 108
Refactor m_riemann_solvers
Module (Non-Solver Subroutines)
#908
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
base: master
Are you sure you want to change the base?
Conversation
PR Reviewer Guide 🔍Here are some key observations to aid the review process:
|
There was a problem hiding this 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 m_riemann_solvers
module by extracting all inline wave‐speed logic into a new s_compute_wave_speed
subroutine, and cleans up buffer initializations with pointers and target attributes.
- Adds
s_compute_wave_speed
(inm_variables_conversion
) to centralize all HLL/HLLC/HLLD wave‐speed formulas. - Updates
m_riemann_solvers.fpp
to calls_compute_wave_speed
instead of duplicating logic, and refactors boundary‐buffer routines usingpointer
/target
. - Adjusts variable declarations (
intent
,allocatable
,pointer
, newbc_beg
,bc_end
,end_val
) for directional BC handling.
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
File | Description |
---|---|
src/simulation/m_riemann_solvers.fpp | Replaced duplicated wave‐speed blocks with calls to s_compute_wave_speed ; refactored Riemann buffer loops with pointers/targets and unified BC handling |
src/common/m_variables_conversion.fpp | Exported and implemented new s_compute_wave_speed subroutine under #ifndef MFC_PRE_PROCESS |
Comments suppressed due to low confidence (2)
src/common/m_variables_conversion.fpp:1714
- There’s no
!>
docstring fors_compute_wave_speed
. Add a header explaining the purpose, inputs, and outputs, so future readers can quickly understand the routine.
subroutine s_compute_wave_speed(wave_speeds, vel_L, vel_R, pres_L, pres_R, rho_L, rho_R, rho_avg, &
src/common/m_variables_conversion.fpp:1714
- The newly introduced
s_compute_wave_speed
contains complex branching for multiple wave‐speed methods but no dedicated unit tests. Consider adding focused tests covering bothwave_speeds==1
andwave_speeds==2
paths.
subroutine s_compute_wave_speed(wave_speeds, vel_L, vel_R, pres_L, pres_R, rho_L, rho_R, rho_avg, &
PR Code Suggestions ✨Latest suggestions up to dd158d0
Previous suggestions✅ Suggestions up to commit b684a4e
|
Good catch, I forgot about it honestly when replacing wave speed calcs with the subroutine call. Thnx. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #908 +/- ##
==========================================
- Coverage 44.15% 44.13% -0.02%
==========================================
Files 68 68
Lines 18347 18218 -129
Branches 2227 2210 -17
==========================================
- Hits 8101 8041 -60
+ Misses 8943 8891 -52
+ Partials 1303 1286 -17 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
be sure to run tests on frontier and phoenix (amd and nvidia gpu) before pushing. helps you debug faster and saves the runners to do other things. |
Roger that |
@@ -1709,4 +1710,118 @@ contains | |||
end subroutine s_compute_fast_magnetosonic_speed | |||
#endif | |||
|
|||
#ifndef MFC_PRE_PROCESS | |||
subroutine s_compute_wave_speed(wave_speeds, vel_L, vel_R, pres_L, pres_R, rho_L, rho_R, rho_avg, & |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
surely some of these arguments should be optional since not every riemann solver uses them all?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yup yup, not all of them will be used in every scenario. I will look into a way to reduce that as the subroutine call looks the same in every solver yet not neat-looking
call s_compute_wave_speed(wave_speeds, vel_L, vel_R, pres_L, pres_R, rho_L, rho_R, rho_avg, &
c_L, c_R, c_avg, c_fast%L, c_fast%R, G_L, G_R, &
tau_e_L, tau_e_R, gamma_L, gamma_R, pi_inf_L, pi_inf_R, &
s_L, s_R, s_S, s_M, s_P, dir_idx(1), dir_idx_tau(1))
@Malmahrouqi3 |
Not frankly sure what is happening. I tried interactive where all tests passed vs. slurm where all tests failed for phoenix gpu. No specific error messages e.g. tolerance issue or such, yet prolly due to the lint issue. I will fix that and re-run the slurm job. |
The error is only on GPU cases and is because you did something with OpenACC incorrectly: FATAL ERROR: data in PRESENT clause was not found on device 1: name=c_fast host:0x3d77820
file:/storage/scratch1/6/sbryngelson3/mfc-runners/actions-runner-03/_work/MFC/MFC/src/simulation/m_riemann_solvers.fpp s_hllc_riemann_solver line:2287 |
@@ -1259,6 +1207,8 @@ contains | |||
|
|||
integer :: i, j, k, l, q !< Generic loop iterators | |||
integer :: idx1, idxi | |||
type(riemann_states) :: c_fast, vel |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i don't think this is right
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added this initially like a while back as it gave me errors and refused to complete the build. More likely the reason tho, I will look into this.
User description
Description
Reduced PR from (#855),
Essentially, I reduced non-math-critical subroutines slightly. In addition, I added s_compute_wave_speed as a comprehensive subroutine to tackle its calcs for all Riemann Solvers (HLL, HLLC, HLLD).
PR Type
Enhancement
Description
Refactored wave speed computation into centralized subroutine
Consolidated Riemann state buffer population logic
Simplified output data reshaping in Riemann solver
Fixed intent declarations and removed allocatable from pointers
Changes diagram
Changes walkthrough 📝
m_variables_conversion.fpp
Add centralized wave speed computation subroutine
src/common/m_variables_conversion.fpp
s_compute_wave_speed
subroutine for centralized wave speedcalculation
support
m_riemann_solvers.fpp
Refactor Riemann solver with centralized functions
src/simulation/m_riemann_solvers.fpp
s_compute_wave_speed
from pointers