Adding (some) pure functions and subroutines #840
Conversation
|
@prathi-wind FYI you're creating more merge conflicts. merge with base branch before continuing (I recommend) |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #840 +/- ##
==========================================
- Coverage 43.48% 43.47% -0.02%
==========================================
Files 68 68
Lines 19778 19766 -12
Branches 2377 2375 -2
==========================================
- Hits 8601 8593 -8
+ Misses 9729 9726 -3
+ Partials 1448 1447 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
sbryngelson
changed the title
There was a problem hiding this comment.
Most of this looks good. I suggested one additional routine that could be made elemental in addition to pure.
There's one set of changes I don't understand, @prathi-wind . You touched a lot of code in m_bubbles.fpp to pass global parameters in as intent(in) arguments. Was this necessary? Since they're all intent(in) the original code should have never written them, so there should have been no side effects inhibiting the pure. Did a compiler refuse to accept the pure elemental without this?
| @@ -54,7 +54,7 @@ end function f_is_default | |||
|
|
|||
| !> Checks if ALL elements of a real(wp) array are of default value. | |||
| !! @param var_array Array to check. | |||
| logical function f_all_default(var_array) result(res) | |||
| logical pure function f_all_default(var_array) result(res) | |||
There was a problem hiding this comment.
Now that f_is_default is elemental you could rewrite the implementation of f_all_default to take advantage of that using all(f_is_default(var_array)) instead of the loop.
Not necessary, but could serve as a bell weather function for how well a given compiler is going to support elemental.
src/pre_process/m_model.fpp
Outdated
| @@ -532,7 +532,7 @@ contains | |||
| !! @param ray Ray. | |||
| !! @param triangle Triangle. | |||
| !! @return True if the ray intersects the triangle, false otherwise. | |||
| function f_intersects_triangle(ray, triangle) result(intersects) | |||
| pure function f_intersects_triangle(ray, triangle) result(intersects) | |||
There was a problem hiding this comment.
This could be made elemental, but I don't know if it would ever be used that way.
There was a problem hiding this comment.
well, i'm curious to see if it works if it could!
src/simulation/m_bubbles.fpp
Outdated
| real(wp), intent(in) :: gam_l, Ca_l, Web_l, Re_inv_l | ||
| logical, intent(in) :: bubbles_euler_l, polytropic_l |
There was a problem hiding this comment.
Was adding these as dummy arguments necessary for the compiler to accept this as a pure elemental procedure? Since these can be made intent(in) they must not have been modified prior to the refactoring, and so the routine should have had no side effects.
There was a problem hiding this comment.
see my comment on slack:
i’m curious what you think is a good strategy here. i think was the one who recommended passing in globals as parameters to a subroutine. i’m not sure if this is required to for a subroutine to be pure or not. it does seem that if they aren’t passed with intent(in) then they could be changed within the routine and thus cause side effects? for what it’s worth - this is partially an effort to use fewer module globals and make subroutines more self-contained, but maybe that’s orthogonal to making things pure or elemental? thoughts?
src/simulation/m_bubbles.fpp
Outdated
| real(wp), intent(in) :: gam_l, Ca_l, Web_l, Re_inv_l | ||
| logical, intent(in) :: polytropic_l |
There was a problem hiding this comment.
Similar to above, since these are intent(in) why was the refactor necessary to make it pure?
src/simulation/m_bubbles.fpp
Outdated
| real(wp), intent(in) :: gam_l, Ca_l, Web_l, Re_inv_l | ||
| logical, intent(in) :: polytropic_l |
There was a problem hiding this comment.
Similar to above, since these are intent(in) why was the refactor necessary to make it pure?
src/simulation/m_bubbles.fpp
Outdated
| !$acc routine seq | ||
| real(wp), intent(in) :: fCpbw, fR, fV, fH, fHdot | ||
| real(wp), intent(in) :: fcgas, fntait, fBtait | ||
| real(wp), intent(in) :: Re_inv_l |
There was a problem hiding this comment.
Similar to above, since these are intent(in) why was the refactor necessary to make it pure?
src/simulation/m_bubbles.fpp
Outdated
| real(wp), intent(in) :: gam_l, Ca_l, Web_l, Re_inv_l | ||
| logical, intent(in) :: polytropic_l |
There was a problem hiding this comment.
Similar to above, since these are intent(in) why was the refactor necessary to make it pure?
src/simulation/m_bubbles.fpp
Outdated
| real(wp), intent(in) :: gam_l, Ca_l, Web_l, Re_inv_l | ||
| logical, intent(in) :: polytropic_l |
There was a problem hiding this comment.
Similar to above, since these are intent(in) why was the refactor necessary to make it pure?
src/simulation/m_bubbles.fpp
Outdated
| real(wp), intent(in) :: Tw_l, R_v_l, pv_l | ||
| real(wp), dimension(:), allocatable, intent(in) :: pb0_l, mass_n0_l, mass_v0_l, Pe_T_l | ||
| real(wp), dimension(:), allocatable, intent(in) :: Re_trans_T_l | ||
| real(wp), intent(in) :: k_mw_l | ||
| integer, intent(in) :: thermal_l | ||
| logical, intent(in) :: bubbles_lagrange_l |
There was a problem hiding this comment.
Similar to above, since these are intent(in) why was the refactor necessary to make it pure?
There was a problem hiding this comment.
This all looks good to me! There's enough changes that I need to trust the tested compilers are doing the work to verify pure and elemental, but I spot-checked a dozen or so an they all look right to me.
I also looked at the few more substantial code changes and they look good.
Much appreciated! Will merge. |
Description
I have marked functions and subroutines as pure and elemental to allow for easier compiler parallelization and better code practice.
Fixes #712
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:
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 --omniperf, and have attached the output file and plain text results to this PR.