Multicomponent diffusion fluxes, thermal conduction, and mixture viscosity #888
Conversation
DimAdam-01
changed the title
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #888 +/- ##
==========================================
- Coverage 45.76% 45.42% -0.34%
==========================================
Files 68 68
Lines 18668 18798 +130
Branches 2251 2266 +15
==========================================
- Hits 8543 8539 -4
- Misses 8767 8892 +125
- Partials 1358 1367 +9 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull Request Overview
This PR integrates multicomponent diffusion fluxes, adds thermal conduction to the energy equation, and computes mixture viscosity throughout the solver.
- Added calls to compute and invert mixture viscosity in the Riemann solver
- Extended the RHS assembly to allocate, compute, and apply chemistry diffusion fluxes
- Introduced
s_compute_chemistry_diffusion_fluxinm_chemistry.fppto calculate species diffusion and thermal conduction
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/simulation/m_riemann_solvers.fpp | Imported get_mixture_viscosity_mixavg, added viscosity logic and zeroing loops for diffusion flux sources |
| src/simulation/m_rhs.fpp | Allocated diffusion buffers, invoked diffusion RHS routines, and merged diffusion into physics RHS |
| src/common/m_chemistry.fpp | Added s_compute_chemistry_diffusion_flux subroutine and updated imports for thermal conduction and diffusivities |
Comments suppressed due to low confidence (2)
src/simulation/m_rhs.fpp:535
- This allocation isn’t paired with a deallocation, risking a memory leak and persistent GPU buffers. Consider deallocating these arrays when no longer needed or using automatic sizing within the data structure.
@:ALLOCATE(flux_src_n(i)%vf(E_idx)%sf( &
src/common/m_chemistry.fpp:13
- The imported
get_species_binary_mass_diffusivitiesis not used in this module. Remove the unused import to keep dependencies minimal.
get_mole_fractions, get_species_binary_mass_diffusivities, &
src/simulation/m_riemann_solvers.fpp
Outdated
| call get_mixture_viscosity_mixavg(T_L, Ys_L, Re_L(1)) | ||
| call get_mixture_viscosity_mixavg(T_R, Ys_R, Re_R(1)) | ||
| Re_L(1) = 1.0_wp/Re_L(1) | ||
| Re_R(1) = 1.0_wp/Re_R(1) |
There was a problem hiding this comment.
[nitpick] The viscosity calculation and inversion is duplicated in multiple solver branches. Refactor this into a helper subroutine to avoid copy–paste and ease future updates.
| call get_mixture_viscosity_mixavg(T_L, Ys_L, Re_L(1)) | |
| call get_mixture_viscosity_mixavg(T_R, Ys_R, Re_R(1)) | |
| Re_L(1) = 1.0_wp/Re_L(1) | |
| Re_R(1) = 1.0_wp/Re_R(1) | |
| call compute_viscosity_and_inversion(T_L, Ys_L, T_R, Ys_R, Re_L, Re_R) |
Copilot uses AI. Check for mistakes.
|
|
||
| !$acc parallel loop collapse(4) gang vector default(present) | ||
| do i = E_idx, chemxe | ||
| do l = is3%beg, is3%end | ||
| do k = is2%beg, is2%end | ||
| do j = is1%beg, is1%end | ||
| if (i == E_idx .or. i >= chemxb) then | ||
| flux_src_vf(i)%sf(j, k, l) = 0._wp |
There was a problem hiding this comment.
[nitpick] The three separate nested loops zeroing out flux_src_vf for each coordinate ordering are nearly identical. Consolidate into a single parametrized kernel or subroutine to reduce duplication and improve readability.
| !$acc parallel loop collapse(4) gang vector default(present) | |
| do i = E_idx, chemxe | |
| do l = is3%beg, is3%end | |
| do k = is2%beg, is2%end | |
| do j = is1%beg, is1%end | |
| if (i == E_idx .or. i >= chemxb) then | |
| flux_src_vf(i)%sf(j, k, l) = 0._wp | |
| !$acc parallel loop collapse(4) gang vector default(present) | |
| do i = E_idx, chemxe | |
| do l = is3%beg, is3%end | |
| do j = is1%beg, is1%end | |
| do k = is2%beg, is2%end | |
| if (i == E_idx .or. i >= chemxb) then | |
| flux_src_vf(i)%sf(k, j, l) = 0._wp |
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
pretty good comments! also you need to add test cases for diffusion, right?
There was a problem hiding this comment.
The deallocation fix and the refactoring is a good catch. The loop structure suggestion is strange because I'm following exactly the same pattern used for the viscous implementation just a few lines above there's no logical reason to change the loop ordering.
Regarding testing, I've already generated the initial test file for diffusion and now I am working on a boundary test case, trying to design a scenario where the method demonstrates its effectiveness within a short timeframe.
There was a problem hiding this comment.
it only needs to be using this feature (diffusion) for a few time steps, long enough that it meaningfully changes the results.
There was a problem hiding this comment.
The loop structure suggestion is strange because I'm following exactly the same pattern used for the viscous implementation just a few lines above there's no logical reason to change the loop ordering.
@DimAdam-01 yes copilot is just pointing is where THIS PR is doing something that is not best practices, it isn't reviewing the entire code (which also has a lot of redundant code)
There was a problem hiding this comment.
@sbryngelson, Ok I checked the golden text file that was produced, and there's only a change at the 4th decimal place. Should I extend the simulation time to see more significant differences?
PR Reviewer Guide 🔍Here are some key observations to aid the review process:
|
User description
Description
Please include a summary of the changes and the related issue(s) if they exist.
This update implements the diffusive fluxes for chemical species, includes thermal conduction in the energy equation, and computes the mixture’s viscosity
Please also include relevant motivation and context.
Fixes #(issue) [optional]
Type of change
Please delete options that are not relevant.
Scope
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?
Please describe the tests that you ran to verify your changes.
Provide instructions so we can reproduce.
Please also list any relevant details for your test configuration
Test Configuration:
My Laptop, Delta (A100) and Delta AI (GH)
Checklist
docs/)examples/that demonstrate my new feature performing as expected.They run to completion and demonstrate "interesting physics"
./mfc.sh formatbefore committing my codeIf your code changes any code source files (anything in
src/simulation)To make sure the code is performing as expected on GPU devices, I have:
nvtxranges so that they can be identified in profiles./mfc.sh run XXXX --gpu -t simulation --nsys, and have attached the output file (.nsys-rep) and plain text results to this PR./mfc.sh run XXXX --gpu -t simulation --rsys --hip-trace, and have attached the output file and plain text results to this PR.PR Type
Enhancement
Description
Implement multicomponent diffusion fluxes for chemical species
Add thermal conduction to energy equation
Integrate mixture viscosity computation for chemistry
Extend RHS computation for diffusion physics
Changes walkthrough 📝
m_chemistry.fpp
Add multicomponent diffusion flux computationsrc/common/m_chemistry.fpp
calculations
s_compute_chemistry_diffusion_fluxsubroutine for speciesdiffusion
m_rhs.fpp
Integrate diffusion physics into RHS computationsrc/simulation/m_rhs.fpp
m_riemann_solvers.fpp
Add mixture viscosity and diffusion flux initializationsrc/simulation/m_riemann_solvers.fpp