Collab/ndrop #172

odiazib · 2023-05-17T21:52:52Z

This PR contains the C++ port of the Fortran subroutine ndrop We port and create validation tests for the following subroutines:

Ccncalc: test for a single cell and another one for a column; passing with a relative error of 1e-5 and 2e-3
get_activate_frac: column test; passing with a relative error of 5e-5
loadaer: single cell test; passing with a relative error of 6e-8
activate_modal: single cell test; passing with a relative error of 7e-5
explmix: column tests; passing with a relative error of 1e-9 and 1e-10
maxsat: single cell test; passing with relative error of 1e-11
update_from_newcld: single cell tests; passing with relative errors of 1e-11, 1e-3, 2e-9
update_from_cldn_profile: single cell tests; passing with relative errors of 1e-12 and 9e-3
dropmixnuc: column tests; passing with relative error of 5e-5 and 6.9e-4
A validation test for update_from_explmix will part of a future PR.

Notes:

The ndrop fortran code involves multiple columns; however, our porting was adapted for one column. As a result, ndrop/mam4xx returns a per cell value for index kk, instead of returning a column-integrated variable ndropcol (column-integrated droplet number).
Instead of allocating temporary arrays in the main driver (dropmixnuc), we pass work arrays as inputs.
We added nested parallelism in dropmixnuc. Therefore, we added team_barriers to synchronize threads and avoid race conditions.
Index-mapping arrays (mam_idx, mam_cnst_idx, numptr_amode, lmassptr_amode, lspectype_amode) are inputs in ndrop/mam4xx. In addition, conversion from Fortran indexing to C++ indexing is done inside of the functions. We did not modify ndrop/mam4 indexing arrays. For future work, we could work on merging these different mapping arrays.
In an effort to understand the index arrays in mam4, we made some notes on Confluence.

Closes #161

codecov · 2023-05-19T23:09:19Z

Codecov Report

Merging #172 (74a244a) into main (4ebe844) will increase coverage by 0.37%.
The diff coverage is 99.32%.

@@            Coverage Diff             @@
##             main     #172      +/-   ##
==========================================
+ Coverage   95.93%   96.30%   +0.37%     
==========================================
  Files          28       28              
  Lines        4626     5202     +576     
==========================================
+ Hits         4438     5010     +572     
- Misses        188      192       +4

Impacted Files	Coverage Δ
src/mam4xx/ndrop.hpp	`99.34% <99.32%> (-0.66%)`	⬇️

jeff-cohere · 2023-07-20T20:18:19Z

src/tests/CMakeLists.txt

@@ -66,7 +66,7 @@ target_compile_options(mam4_nucleate_ice_unit_tests PRIVATE -Werror)
 # TODO: return to this test once we have more context for get_aer_num
 EkatCreateUnitTest(mam4_ndrop_unit_tests mam4_ndrop_unit_tests.cpp
  LIBS mam4xx_tests ${HAERO_LIBRARIES} EXCLUDE_TEST_SESSION)
-target_compile_options(mam4_ndrop_unit_tests PRIVATE -Werror)
+target_compile_options(mam4_ndrop_unit_tests PRIVATE)


You can delete this line, which no longer does anything.

jeff-cohere · 2023-07-20T20:25:25Z

src/validation/ndrop/CMakeLists.txt

+               maxsat.cpp
+               update_from_newcld.cpp
+               update_from_cldn_profile.cpp
+              #  update_from_explmix.cpp


Is update_from_explmix still in progress? I noticed a comment in that file related to a thread team.

The validation test is still in progress, but the subroutine was ported. We could remove this code from this PR.

It's fine to leave the code in with this comment. I remember you and Mike telling me about this now.

jeff-cohere · 2023-07-20T20:33:14Z

src/validation/ndrop/ndrop_driver.cpp

+      //  update_from_explmix(ensemble);
+    } else if (func_name == "dropmixnuc") {
+      dropmixnuc(ensemble);
+    }


It seems like this code will silently do nothing if the function name doesn't match any of the names above. Might be good to put in an } else { block to make sure that doesn't happen.

This if statement is inside of a try statement that print an error.

auto func_name = settings.get("function"); try { if { } else if { }... ... } catch (std::exception &e) { std::cerr << argv[0] << ": Error: " << e.what() << std::endl; }

True, but if func_name doesn't match any of the names in the if tests, no exception is thrown, right?

You are correct. I will fix this.

jeff-cohere · 2023-07-20T20:42:06Z

src/tests/mam4_ndrop_unit_tests.cpp

+                                           0.1999170945E005, 0.9524671355E005};
+
+  for (int i = 0; i < AeroConfig::num_modes(); ++i) {
+    ss << "i = " << i;


Not a big deal, but you might find the fmt library a little more convenient for creating log messages. EKAT provides fmt in addition to our logging library, so feel free to use it.

jeff-cohere · 2023-07-20T20:49:06Z

src/mam4xx/ndrop.hpp

 #include <haero/math.hpp>

 #include <mam4xx/aero_config.hpp>
 #include <mam4xx/conversions.hpp>
 #include <mam4xx/mam4_types.hpp>
+#include <mam4xx/ndrop.hpp>


Is this file including itself?

Slightly related: can you add a line to src/CMakeLists to make sure that the ndrop.hpp header is installed when we run make install?

jeff-cohere · 2023-07-20T20:59:54Z

src/mam4xx/ndrop.hpp

+  const Real pi = haero::Constants::pi;
+
+  // surface tension coefficient [m-K] (from mam4)
+  // FIXME: any thoughts on what [m-K] means?


In my experience, fit coefficients often can't be interpreted physically, at least not directly. It seems to me that ssat_coeff captures a power-law temperature dependence which is expressed in the form of this "surface tension" component. Note also the headache-inducing fractional-power (power law) units of ssat_coeff.

jeff-cohere · 2023-07-20T21:02:23Z

src/mam4xx/ndrop.hpp

+    // critical supersaturation at mode radius [fraction]
+    // here we assume "value shouldn't matter much since naerosol is small"
+    // ^(from mam4)
+    // NOTE: excuse me... what??


src/mam4xx/ndrop.hpp

jeff-cohere · 2023-07-20T21:12:20Z

src/mam4xx/ndrop.hpp

+  // @param [out]   flux_fullact flux of activated aerosol fraction assuming
+  // 100% activation [m/s]
+  const Real zero = 0;
+  const int nmodes = AeroConfig::num_modes();


Should this (and statements like it) be constexpr int instead of const int to make sure that it can be used to provide a size for automatic arrays?

jeff-cohere

I took a pass and left some minor comments. It looks really good! Nice work on this.

pressel

Thanks to all who worked on this, it looks great. I am fine with merging this now.

…mr_sum

… not match any of the function names in the validation driver.

…ing.

odiazib closed this May 18, 2023

odiazib reopened this May 19, 2023

mjs271 force-pushed the collab/ndrop branch from afdb4af to de814d1 Compare June 15, 2023 22:42

odiazib force-pushed the collab/ndrop branch from 1b209e6 to cfcb042 Compare July 4, 2023 15:58

odiazib marked this pull request as ready for review July 4, 2023 16:05

overfelt approved these changes Jul 6, 2023

View reviewed changes

mjs271 force-pushed the collab/ndrop branch from 71280db to 8d3d661 Compare July 18, 2023 20:33

odiazib force-pushed the collab/ndrop branch 2 times, most recently from 08a1621 to d588e05 Compare July 20, 2023 13:34

odiazib requested review from jeff-cohere, singhbalwinder, pressel, mjs271, brigaudet and jaelynlitz July 20, 2023 20:10

jeff-cohere reviewed Jul 20, 2023

View reviewed changes

src/mam4xx/ndrop.hpp Show resolved Hide resolved

jeff-cohere reviewed Jul 20, 2023

View reviewed changes

jeff-cohere approved these changes Jul 20, 2023

View reviewed changes

jeff-cohere mentioned this pull request Jul 20, 2023

verification/validation tests for ndrop explmix #147

Open

pressel approved these changes Jul 20, 2023

View reviewed changes

odiazib added 2 commits July 21, 2023 09:09

ndrop - initial port for ccncalc, loadaer, get_aer_num, and get_aer_m…

39d7167

…mr_sum

ndrop - activate_modal

87157d5

odiazib added 26 commits July 21, 2023 09:09

ndrop - clang format

7c4ce3d

ndrop - 2d view for nact

5a7cb82

ndrop - 2d view for mact

9d05076

ndrop - delete work array (qcldbrn_num)

0057f30

ndrop - cleaning dropmixnuc validation test

89f72a8

ndrop - cleaning validation tests

6eba0d8

ndrop - 1. Using a 2d view for ccn. 2. Cleaning ccncalc validation test.

07973e4

ndrop - cleaning ccncalc_single_cell and explmix

f15067f

ndrop - cleaning get_activate_frac test

ffa19f2

ndrop - additional cleaning for get_activate_frac

5f7fb80

ndrop - save

3a0f455

ndrop - cleaning loadaer

b3f68fb

nndrop - clang format

db32c1b

ndrop - cleaning update_from_cldn_profile

6044393

ndrop - more cleaning

50dba21

ndrop - clang format

4a335bd

ndrop - fix FIXME and clang format

aaef967

ndrop - validation tests

8f6fc8e

ndrop - error tolerances

1f62913

ndrop - update relative errors

d864a16

ndrop - Add error if a validation test has an function name that does…

8fc5f93

… not match any of the function names in the validation driver.

ndrop - remove header

51d5453

ndrop - update comments

8b8c650

ndrop - switch const to constexpr

69229ee

ndrop - delete old comments and code line, which no longer does anyth…

f2a43f7

…ing.

ndrop - clang format

74a244a

odiazib force-pushed the collab/ndrop branch from 699357a to 74a244a Compare July 21, 2023 15:35

odiazib merged commit 1068da0 into main Jul 21, 2023
7 checks passed

odiazib deleted the collab/ndrop branch July 21, 2023 22:13

mjs271 mentioned this pull request Aug 18, 2023

swap to {fmt} syntax in ndrop unit test #228

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Collab/ndrop #172

Collab/ndrop #172

odiazib commented May 17, 2023 •

edited

Loading

codecov bot commented May 19, 2023 •

edited

Loading

jeff-cohere Jul 20, 2023

jeff-cohere Jul 20, 2023

odiazib Jul 20, 2023

jeff-cohere Jul 20, 2023

jeff-cohere Jul 20, 2023

odiazib Jul 20, 2023

jeff-cohere Jul 20, 2023

odiazib Jul 20, 2023

odiazib Jul 21, 2023

jeff-cohere Jul 20, 2023

jeff-cohere Jul 20, 2023

odiazib Jul 21, 2023

jeff-cohere Jul 20, 2023

jeff-cohere Jul 20, 2023

jeff-cohere Jul 20, 2023

jeff-cohere left a comment

pressel left a comment

Collab/ndrop #172

Collab/ndrop #172

Conversation

odiazib commented May 17, 2023 • edited Loading

Notes:

codecov bot commented May 19, 2023 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeff-cohere left a comment

Choose a reason for hiding this comment

pressel left a comment

Choose a reason for hiding this comment

odiazib commented May 17, 2023 •

edited

Loading

codecov bot commented May 19, 2023 •

edited

Loading