-
Notifications
You must be signed in to change notification settings - Fork 6
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
Coagulation #120
Coagulation #120
Conversation
@cameronrutherford @overfelt Just letting you both know about this Draft PR so you can keep your eyes on it. |
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.
This looks pretty good to me. I left a few minor comments. Also: Do we need an empty wet_density.hpp
file?
@pressel , can you update the description for this PR with any relevant information, including anything that you'd like reviews to give more focused attention?
const auto tend_qgas0 = tends.q_gas[0]; | ||
auto h_prog_qgas0 = Kokkos::create_mirror_view(prog_qgas0); | ||
auto h_tend_qgas0 = Kokkos::create_mirror_view(tend_qgas0); | ||
Kokkos::deep_copy(h_prog_qgas0, prog_qgas0); |
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.
Check this deep_copy:
data from device is copied to host, but I believe you need to copy from host to device. Also, values in device are initialize to zeros.
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.
@jeff-cohere Do you want to take a look at this, I essentially copied this bit of code from mam4_nucleation_unit_tests.cpp.
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.
Maybe it would be more instructive to copy test code from calcsize
or gasaerexch
or another process that uses several aerosol species. The nucleation parameterization was the first one we attacked and it only really adds sulfate to the aitken mode. And the testing on the other parameterizations is definitely more thorough at this point--I'm going to have to revisit nucleation and raise it to the standard set by others!
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.
In consultation with @jeff-cohere I'm going to opt for improving the testing beyond a GPU smoke tests in another PR.
const int max_agepair = Coagulation::max_agepair; | ||
Real qaer_cur_c[num_aero][num_modes]; | ||
int n = 0; | ||
for (int imode = 0; imode < num_modes; ++imode) { |
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.
check validation::convert_vector_to_mass_mixing_ratios
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 directly ported https://github.com/eagles-project/e3sm_mam4_refactor/blob/refactor-maint-2.0/components/eam/src/chemistry/modal_aero/modal_aero_coag.F90
There the various qaer arrays are transposed from what is assumed in validation::convert_vector_mass_mixing ratios.
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 switched q_mmr[ispec][imode] to q_mmr[imode] [ispec] to be consistent with the prognostic variables in aero_config.hpp
Doing this change will require to modified the order of imode and ispec in the code.
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'm going to add a validation convenience function similar to validation::convert_mass_mixing_ratios_to_vector but that properly accounts of the transposition of mode and aerosol dimensions that is used in coagulation, aging, and gas aerosol exchange processes (I'd imagine similar transpositions occur in other parts of the code as well). I will add that function as part of another PR.
qaer_del_coag_out_c); | ||
|
||
n = 0; | ||
for (int imode = 0; imode < num_modes; ++imode) { |
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.
check validation::convert_mass_mixing_ratios_to_vector
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 directly ported https://github.com/eagles-project/e3sm_mam4_refactor/blob/refactor-maint-2.0/components/eam/src/chemistry/modal_aero/modal_aero_coag.F90
There the various qaer arrays are transposed from what is assumed in validation::convert_vector_mass_mixing ratios.
const Real bijqnumj2 = haero::max(0.0, ybetaij3[2] * qnum_tavg[npca]); | ||
Real decay_const = bijqnumj1 + bijqnumj2; | ||
|
||
constexpr float epsilonx2 = std::numeric_limits<float>::epsilon() * 2.0; |
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.
Are we allow to use std functions in CUDA?
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.
@jeff-cohere and I talked about handling this numerical limit, and I think because it is being used as a constexper it is ok. @jeff-cohere correct me if I am wrong.
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.
Well, I don't know that this is supposed to work--the std
namespace is not available to CUDA C++ as far as I know. Can someone try to build this on a GPU workstation available to them?
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.
The std::numeric_limits() compiles and the tests pass with Cuda 11.2 and GCC 10.3 on Linux.
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.
Well, that's interesting, though it doesn't give me a very warm and fuzzy feeling! I guess we'll find out how intentional this is as we build on more exotic platforms.
Thanks for testing, James.
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 believe there is an equivalent version of std::numeric_limits::epsilon() in kokkos.
@jeff-cohere I removed wet_density.hpp. |
Codecov Report
@@ Coverage Diff @@
## main #120 +/- ##
===========================================
- Coverage 91.54% 74.92% -16.63%
===========================================
Files 54 27 -27
Lines 4529 2823 -1706
===========================================
- Hits 4146 2115 -2031
- Misses 383 708 +325
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
This pull request ports the MAM4 representation of Coagulation from Fortran to C++. The refactored Fortran source upon which this port is based can be found here.
A few comments on this PR that mostly arise due to the fact that we have maintained a direct port of the F90 code:
Note that 2D arrays with dimensions of
num_aer
andnum_mode
are transposed from the order assumed by haero. For example in Coagulationqaer
arrays are dimensioned with as[num_aer][num_mode]
, where as within haeroqaer
arrays would be dimensioned as[num_mode][num_aer]
. The transposition of indices is consistent with Aging and Gas Aerosol Exchange processes.Like Aging and Gas Aerosol Exchange, Coagulation accesses array elements in
qaer_cur
that strictly speaking shouldn't exist as prognostic variables within MAM4. It is my understanding that these array elements are being used by MAM4 temporary storage for values that are used later by other processes. We must thus be careful account for this at the level of the source code where these processes are called.