-
Notifications
You must be signed in to change notification settings - Fork 55
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
Adds MAM4xx microphysics interface #3013
base: master
Are you sure you want to change the base?
Adds MAM4xx microphysics interface #3013
Conversation
Status Flag 'Pull Request AutoTester' - User Requested Retest - Label AT: RETEST will be reset after testing. |
Status Flag 'Pre-Test Inspection' - - This Pull Request Requires Inspection... The code must be inspected by a member of the Team before Testing/Merging |
Status Flag 'Pull Request AutoTester' - User Requested Retest - Label AT: RETEST will be reset after testing. |
Status Flag 'Pre-Test Inspection' - - This Pull Request Requires Inspection... The code must be inspected by a member of the Team before Testing/Merging |
Status Flag 'Pull Request AutoTester' - User Requested Retest - Label AT: RETEST will be reset after testing. |
Status Flag 'Pre-Test Inspection' - SUCCESS: The last commit to this Pull Request has been INSPECTED by label AT: PRE-TEST INSPECTED! Autotester is Removing Label; this inspection will remain valid until a new commit to source branch is performed. |
Status Flag 'Pull Request AutoTester' - Testing Jenkins Projects: Pull Request Auto Testing STARTING (click to expand)Build InformationTest Name: SCREAM_PullRequest_Autotester_Weaver
Jenkins Parameters
Build InformationTest Name: SCREAM_PullRequest_Autotester_Mappy
Jenkins Parameters
Using Repos:
Pull Request Author: odiazib |
Status Flag 'Pull Request AutoTester' - Jenkins Testing: 1 or more Jobs FAILED Note: Testing will normally be attempted again in approx. 2 Hrs. If a change to the PR source branch occurs, the testing will be attempted again on next available autotester run. Pull Request Auto Testing has FAILED (click to expand)Build InformationTest Name: SCREAM_PullRequest_Autotester_Weaver
Jenkins Parameters
Build InformationTest Name: SCREAM_PullRequest_Autotester_Mappy
Jenkins Parameters
SCREAM_PullRequest_Autotester_Weaver # 6078 FAILED (click to see last 100 lines of console output)
SCREAM_PullRequest_Autotester_Mappy # 5849 FAILED (click to see last 100 lines of console output)
|
Status Flag 'Pull Request AutoTester' - User Requested Retest - Label AT: RETEST will be reset after testing. |
Status Flag 'Pull Request AutoTester' - Testing Jenkins Projects: Pull Request Auto Testing STARTING (click to expand)Build InformationTest Name: SCREAM_PullRequest_Autotester_Weaver
Jenkins Parameters
Build InformationTest Name: SCREAM_PullRequest_Autotester_Mappy
Jenkins Parameters
Using Repos:
Pull Request Author: odiazib |
Status Flag 'Pull Request AutoTester' - User Requested Retest - Label AT: RETEST will be reset after testing. |
Status Flag 'Pre-Test Inspection' - - This Pull Request Requires Inspection... The code must be inspected by a member of the Team before Testing/Merging |
Status Flag 'Pull Request AutoTester' - User Requested Retest - Label AT: RETEST will be reset after testing. |
Status Flag 'Pre-Test Inspection' - - This Pull Request Requires Inspection... The code must be inspected by a member of the Team before Testing/Merging |
Status Flag 'Pull Request AutoTester' - User Requested Retest - Label AT: RETEST will be reset after testing. |
Status Flag 'Pre-Test Inspection' - SUCCESS: The last commit to this Pull Request has been INSPECTED by label AT: PRE-TEST INSPECTED! Autotester is Removing Label; this inspection will remain valid until a new commit to source branch is performed. |
Status Flag 'Pull Request AutoTester' - User Requested Retest - Label AT: RETEST will be reset after testing. |
Status Flag 'Pull Request AutoTester' - Testing Jenkins Projects: Pull Request Auto Testing STARTING (click to expand)Build InformationTest Name: SCREAM_PullRequest_Autotester_Weaver
Jenkins Parameters
Build InformationTest Name: SCREAM_PullRequest_Autotester_Mappy
Jenkins Parameters
Using Repos:
Pull Request Author: odiazib |
Status Flag 'Pull Request AutoTester' - Jenkins Testing: 1 or more Jobs FAILED Note: Testing will normally be attempted again in approx. 2 Hrs. If a change to the PR source branch occurs, the testing will be attempted again on next available autotester run. Pull Request Auto Testing has FAILED (click to expand)Build InformationTest Name: SCREAM_PullRequest_Autotester_Weaver
Jenkins Parameters
Build InformationTest Name: SCREAM_PullRequest_Autotester_Mappy
Jenkins Parameters
SCREAM_PullRequest_Autotester_Weaver # 6156 FAILED (click to see last 100 lines of console output)
SCREAM_PullRequest_Autotester_Mappy # 5912 FAILED (click to see last 100 lines of console output)
|
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 think this PR has some unrelated edits that could've bene excised to make the PR review process easier. Evidently, the PR isn't really ready. For example, the PR can't be ready with an edit in share/io, that will have to be reverted.
I have written down some comments; I don't think you need to address all of them right now. Things become tedious when a file goes above a few 100s lines; the main addition in this PR is 2356 lines; the (likely misleadingly named) "helper micro" file is 722 lines.
I will review again soon
<mam4_num_a2_verti_emiss_file_name hgrid="ne4np4.pg2" type="file" doc="">${DIN_LOC_ROOT}/atm/scream/mam4xx/emissions/ne4pg2/elevated/cmip6_mam4_num_a2_elev_1x1_2010_clim_ne4pg2_c20241008.nc </mam4_num_a2_verti_emiss_file_name> | ||
<mam4_num_a4_verti_emiss_file_name hgrid="ne4np4.pg2" type="file" doc="">${DIN_LOC_ROOT}/atm/scream/mam4xx/emissions/ne4pg2/elevated/cmip6_mam4_num_a4_elev_1x1_2010_clim_ne4pg2_c20241008.nc </mam4_num_a4_verti_emiss_file_name> | ||
<mam4_soag_verti_emiss_file_name hgrid="ne4np4.pg2" type="file" doc="">${DIN_LOC_ROOT}/atm/scream/mam4xx/emissions/ne4pg2/elevated/cmip6_mam4_soag_elev_1x1_2010_clim_ne4pg2_c20241008.nc </mam4_soag_verti_emiss_file_name> | ||
</mam4_aero_microphys> |
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.
most added files have space at the end of their names, e.g., ...0241008.nc </ma...
). Not sure if that matters, but just letting you know
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.
Thanks for noticing that. It didn't matter in our tests so far, but we will remove 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.
I think this is irrelevant. I think we strip spaces (and newline chars) when we parse the xml, to avoid errors/surprises. So feel free to use the formatting that makes it easier to visually parse the file.
@@ -42,6 +42,7 @@ add_subdirectory(${EXTERNALS_SOURCE_DIR}/mam4xx ${CMAKE_BINARY_DIR}/externals/ma | |||
# EAMxx mam4xx-based atmospheric processes | |||
add_library(mam | |||
eamxx_mam_microphysics_process_interface.cpp | |||
${SCREAM_BASE_DIR}/src/physics/rrtmgp/shr_orb_mod_c2f.F90 |
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.
would be good to explain why this is needed, especially that it is a F90 file. Don't we have this already in 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.
We followed RRTMGP (see here)
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.
Yeah, I don't think we converted this.
@@ -54,7 +55,7 @@ target_include_directories(mam PUBLIC | |||
${EXTERNALS_SOURCE_DIR}/haero | |||
${EXTERNALS_SOURCE_DIR}/mam4xx/src | |||
) | |||
target_link_libraries(mam PUBLIC physics_share scream_share mam4xx haero) | |||
target_link_libraries(mam PUBLIC physics_share csm_share scream_share mam4xx haero) |
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.
An example of something that could've been handled by another PR to make this PR more focused and leaner.
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.
Again, we followed RRTMGP here for now.
|
||
auto nondim = ekat::units::Units::nondimensional(); | ||
constexpr auto nondim = ekat::units::Units::nondimensional(); |
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.
we usually do const auto
for these, not sure which is better. I personally try to steer clear from constexpr.
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.
C++ folks can chime in. I like constexpr
if it is supported as it makes the code more readable. Seeing constexpr
tells me that this is a compile-time constant and is not set during the runtime.
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.
Either way is fine.
FieldLayout scalar4d_layout_mid = | ||
make_layout({ncol_, num_aero_modes, nlev_}, {"COL", "NMODES", "LEV"}); | ||
// Number of modes | ||
constexpr int nmodes = mam4::AeroConfig::num_modes(); |
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.
constexpr again, luca may better comment on this
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.
Same as above. constexpr
indicates a compile-time constant.
// partially cloudy grid cells, hence both fclea and fcldy are assumed to | ||
// be nonzero. | ||
|
||
constexpr Real eps = 1.e-35; // BAD CONSTANT |
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.
yeah... why not use one of those max limits things?
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.
Currently, we are obsessed with keeping the answers as similar as we can. With all this code, we are still able to match very closely to double-precision answers from the Fortran codebase! We always add BAD CONSTANTS
comment so that we can revisit these later. We may use numeric limits here from C++ in the future.
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 was an absolute doozy. High-level recommendations:
- use more human readable variable names
- split stuff out of this file. Could be nice to have subfiles with all these different routines; from a quick glance, it doesn't loook like they all need to be in this very file
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.
We intend to move this file to MAM4xx repo. @odiazib asked about it today itself! We will move these codes so reviewers don't need to review them. We are working on some other important tasks, so it might be an additional PR to move it to MAM4xx.
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.
helper micro may be an inaccurate name for this file; more like tracer data utils? Either way, I think we should refactor this and move it to somewhere in share in the future; I suspect spa, radiation, and share common needs when it comes to these tracers
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.
That's an excellent point. We are borrowing SPA codes for reading files. In my next PR for reading fractional land use, I am working on creating a common framework so that we don't repeat the code. Since we are short on time, I am planning to create this common framework slowly, piece by piece. So you will see a small step towards this in my next PR.
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.
Actually, unrelated, but I was recently looking at the tracers code in EAM F90. If you don't wanna bother with this in EAMxx, it may be a bit easier to house it in MAM4xx. I am not so sure though ... will keep thinking
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 think the ability to read files should be an EAMxx capability. We should have an epic on it to make it easy and consistent. Tracer data codes in EAM have evolved over the years and can handle a lot of different scenarios. It would be great to have those capabilities in EAMxx.
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.
We decided early in the porting process that IO would be handled by EAMxx. The part of the code relies on the scream-scorpio-interface and the AtmosphereInput class. We do not have access to these features in MAM4xx. I tried to create code that can handle many types of NC files, similar to the code in EAM F90, but I did not want to create a direct port. Instead, I implemented code for three types of files that are currently needed in microphysics.
Indeed, we must find a better name for this file. Thanks for pointing this out.
Looking forward to seeing comments/recommendations from @bartgol, @tcclevenger, @AaronDonahue, who have more experience on this topic.
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.
Yes, this is a task that has been on my todo list for a while. I want a common infrastructure for reading time series from file (possibly doing vertical/horizontal interpolation) and interpolating to the current time stamp. Right now, we have a few places that implement the same thing: spa, iop, mam, and nudging. They all have slightly different assumptions (e.g. regarding whether the input data can contain fillValue kind of data or not), but there's no reason why we should not be able to handle it internally.
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.
@odiazib I will find a time to review early next week.
// The following variables are chemistry mechanism dependent! See | ||
// mam4xx/src/mam4xx/gas_chem_mechanism.hpp) |
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 hoping one day we can fix these names
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 file will also be moved to MAM4xx. We may move everything in impl
directory to MAM4xx.
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.
we add dosc under components/eamxx/docs
@singhbalwinder - thanks for letting us know that the PR is ready to be reviewed! @mahf708 and @kaizhangpnl have already begun reviewing the code. When you have a chance, could you add the results from key process-level evaluations performed by the EAGLES eval team in the PR description so that those results are also documented? @bartgol and @AaronDonahue - can you help with the technical review (and/or assign someone appropriate)? Since this PR is quite large, but we also want to help expedite the review process, I'm also wondering if we may need to touch base again about the level of review needed here. For example, Kai estimates he would need another two weeks of concentrated effort to fully review all of the code this PR; however, the initial results of physical tests that he went over with me today all look good in terms of the expected behaviors being reproduced. Perhaps we can consider doing a "light" review at this stage to enable Balwinder and his team to get the code in place, and then a more thorough review in the immediate future? |
Thanks, Naser, for the quick review and for bringing this up. We followed RRTMGP (see here) for this function. We also would like to implement a C++ solution and will reimplement this function once it becomes available. We should revisit it after merging this PR to fix it both in this process and RRTMGP. We are open to implementing if there exists a better solution at this point. I will address the rest of your comments inline. |
Thanks @susburrows. I will update the PR description with the evaluation results and the rest of the required info.
I agree this PR is quite large, and I truly appreciate and am thankful for the effort and time that goes into reviewing this PR. I am listing some routines below that the reviewers can skip if they want to. These will go into the MAM4xx repo in the future. We just changed the layout for a variable in the following file along with declaring some vars constant in the following file: I can give a code walk-through as well if that is helpful. |
Thanks for the clarification re the shr_orb_mod_c2f piece. In the above line, I actually meant the share/io one in components/eamxx/src/share/io/scream_scorpio_interface.cpp. |
That is an easy one to fix! I will remove it in my next set of changes. |
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 have some suggestions.
On a side note: this PR is way too large for reviewing it. I skimmed through it, cause I can't spend time to understand if what it does is likely to be what was intended, as it would take me a week.
I also don't see any unit test for mam4xx, and there are tons of individual functions. The system tests in eamxx/tests
are of course great, but it's hardly better than a full CIME tests. When things break, we have no way to narrow down which part of mam4xx is responsible.
I know you guys are on a schedule, but please consider:
- adding unit tests for all functions in your impl folder
- break PR into smaller ones
I know the latter may be hard, but I would take even a tiny PR that simply adds utility fcns as an intermediate step, to reduce the number of lines to review in the main PR. With PRs this size, you can't expect a great quality of the review, as the attention span falls drastically after 500 lines or so.
@@ -249,6 +249,55 @@ be lost if SCREAM_HACK_XML is not enabled. | |||
<cldFraction inherit="atm_proc_base"/> | |||
|
|||
<!-- MAM4xx namelist options --> | |||
|
|||
<!-- MAM4xx-Aerosol-Microphysics --> | |||
<mam4_aero_microphys inherit="atm_proc_base"> |
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.
Any change you can add some doc strings for these parameters?
<!--Vertical emissions--> | ||
<verti_emiss_ymd type="integer"> 20100101 </verti_emiss_ymd> | ||
<!-- For all other grids --> | ||
<mam4_so2_verti_emiss_file_name type="file" doc="">${DIN_LOC_ROOT}/atm/scream/mam4xx/emissions/ne30pg2/elevated/cmip6_mam4_so2_elev_1x1_2010_clim_ne30pg2_c20241008.nc </mam4_so2_verti_emiss_file_name> |
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 you planning to use ne30 data for all runs, including ne4 runs? Currently, the coarsening/remapping infrastructure assumes that the fine grid is the model grid, so you can only refine input data (and coarsen outputs). I've been thinking for a while to extend horiz remap to allow reading in data from a finer grid, but it was never really needed. If you do need it though, we can try to bump up the priority.
<mam4_num_a2_verti_emiss_file_name hgrid="ne4np4.pg2" type="file" doc="">${DIN_LOC_ROOT}/atm/scream/mam4xx/emissions/ne4pg2/elevated/cmip6_mam4_num_a2_elev_1x1_2010_clim_ne4pg2_c20241008.nc </mam4_num_a2_verti_emiss_file_name> | ||
<mam4_num_a4_verti_emiss_file_name hgrid="ne4np4.pg2" type="file" doc="">${DIN_LOC_ROOT}/atm/scream/mam4xx/emissions/ne4pg2/elevated/cmip6_mam4_num_a4_elev_1x1_2010_clim_ne4pg2_c20241008.nc </mam4_num_a4_verti_emiss_file_name> | ||
<mam4_soag_verti_emiss_file_name hgrid="ne4np4.pg2" type="file" doc="">${DIN_LOC_ROOT}/atm/scream/mam4xx/emissions/ne4pg2/elevated/cmip6_mam4_soag_elev_1x1_2010_clim_ne4pg2_c20241008.nc </mam4_soag_verti_emiss_file_name> | ||
</mam4_aero_microphys> |
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 think this is irrelevant. I think we strip spaces (and newline chars) when we parse the xml, to avoid errors/surprises. So feel free to use the formatting that makes it easier to visually parse the file.
@@ -42,6 +42,7 @@ add_subdirectory(${EXTERNALS_SOURCE_DIR}/mam4xx ${CMAKE_BINARY_DIR}/externals/ma | |||
# EAMxx mam4xx-based atmospheric processes | |||
add_library(mam | |||
eamxx_mam_microphysics_process_interface.cpp | |||
${SCREAM_BASE_DIR}/src/physics/rrtmgp/shr_orb_mod_c2f.F90 |
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.
Yeah, I don't think we converted this.
// mid points | ||
FieldLayout scalar3d_layout_mid{{COL, LEV}, {ncol_, nlev_}}; | ||
// Layout for 2D (2d horiz) variable | ||
const FieldLayout scalar2d = grid_->get_2d_scalar_layout(); |
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.
Thanks for making this change. Since new processes are often started copy/pasting existing ones, it's good to show the best practices. Here, I strongly encourage to use the grid interfaces to retrieve "standard" layouts.
} // iaer | ||
} // iscldy_subarea | ||
|
||
Real qgas_delaa[max_gas()][nqtendaa()] = {}; |
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.
For clarity: the ={}
part initializes all entries to 0. If you don't need to init to 0, consider removing that part.
// calculates qaerwat using the grid-cell mean interstital-aerosol | ||
// mix-rates and the clear-area RH. aerosol water mixing ratios (mol/mol) | ||
Real qaerwatsub3[num_modes][maxsubarea()]; | ||
assign_2d_array(num_modes, maxsubarea(), 0.0, // in |
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.
You can add = {0}
above, and do away with this call. It's likely to be faster too.
Real dgn_a[num_modes] = {}; | ||
Real dgn_awet[num_modes] = {}; | ||
Real wetdens[num_modes] = {}; | ||
assign_1d_array(num_modes, 0.0, // in |
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.
Pointless assignments.
// out | ||
qgcm_tendaa, qqcwgcm_tendaa); | ||
|
||
#if 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.
Let's not add code to master that has #if 0
. If you don't need it, remove 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.
If you want to use it in debug mode, use more descriptive macros. E.g.,
// For developers. uncomment this to trigger extra debug.
//#define MAM4_EXTRA_DEBUG
// later
#ifdef MAM4_EXTRA_DEBUG
...
#endif
EKAT_REQUIRE_MSG (it.second.num_customers==0, | ||
"Error! ScorpioSession::finalize called, but a file is still in use elsewhere.\n" | ||
" - filename: " + it.first + "\n"); | ||
} | ||
}*/ |
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.
We can add an extra arg to finalize_subsystem:
void finalize_subsystem (bool inside_exception_handling) {
...
for (auto& it : s.files) {
if (it.second.num_customers!=0){
if (inside_exception_handling) {
// just print a message
} else {
EKAT_ERROR_MSG (...);
}
}
}
}
We may have to also pass this bool to AtmosphereDriver::finalize
: it's false in general, but true when inside here.
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.
Thanks for this PR! It is a huge effort. This PR includes code changes not only in the MAMxx/EAMxx interface but also in the MAMxx repo (see below). I am a beginner in C++, so it will take me quite some time to carefully review all the code changes. Given the time sensitivity, I fully support the idea for a "light" review at this moment, but I hope by Dec we can add additional diagnostics of budget terms/process rates in subsequent PRs, so that it's easier to catch any potential bug/problem that might exist.
I am testing the code on Chrysalis and will check if the results are consistent with those obtained from other colleagues. Once it's done, I would update the review checklist.
Code changes in MAMxx repo:
https://github.com/eagles-project/mam4xx/tree/main/src/mam4xx/aero_model_emissions.hpp
https://github.com/eagles-project/mam4xx/tree/main/src/mam4xx/aero_modes.hpp
https://github.com/eagles-project/mam4xx/tree/main/src/mam4xx/aging.hpp
https://github.com/eagles-project/mam4xx/tree/main/src/mam4xx/coagulation.hpp
https://github.com/eagles-project/mam4xx/tree/main/src/mam4xx/conversions.hpp
https://github.com/eagles-project/mam4xx/tree/main/src/mam4xx/gas_chem.hpp
https://github.com/eagles-project/mam4xx/tree/main/src/mam4xx/gas_chem_mechanism.hpp
https://github.com/eagles-project/mam4xx/tree/main/src/mam4xx/gasaerexch.hpp
https://github.com/eagles-project/mam4xx/tree/main/src/mam4xx/gasaerexch_soaexch.hpp
https://github.com/eagles-project/mam4xx/tree/main/src/mam4xx/lin_strat_chem.hpp
https://github.com/eagles-project/mam4xx/tree/main/src/mam4xx/mam4.hpp
https://github.com/eagles-project/mam4xx/tree/main/src/mam4xx/mo_drydep.hpp
https://github.com/eagles-project/mam4xx/tree/main/src/mam4xx/mo_photo.hpp
https://github.com/eagles-project/mam4xx/tree/main/src/mam4xx/mo_setext.hpp
https://github.com/eagles-project/mam4xx/tree/main/src/mam4xx/mo_setinv.hpp
https://github.com/eagles-project/mam4xx/tree/main/src/mam4xx/mo_setsox.hpp
https://github.com/eagles-project/mam4xx/tree/main/src/mam4xx/nucleation.hpp
https://github.com/eagles-project/mam4xx/tree/main/src/mam4xx/rename.hpp
https://github.com/eagles-project/mam4xx/tree/main/src/mam4xx/seq_drydep.hpp
https://github.com/eagles-project/mam4xx/tree/main/src/mam4xx/vertical_interpolation.hpp
|
||
const auto extfrc_icol = ekat::subview(extfrc, icol); | ||
|
||
mam4::mo_setext::extfrc_set(forcings_in, extfrc_icol); |
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.
Would be nice to add a comment here and say if we want to turn off the elevated emissions, then comment out this line.
// aqueous chemistry ... | ||
const Real mbar = haero::Constants::molec_weight_dry_air; | ||
constexpr int indexm = mam4::gas_chemistry::indexm; | ||
mam4::mo_setsox::setsox_single_level( |
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.
Would be nice to add a note here and say if we want to turn off the aqueous phase chemistry, comment out this line.
<dgnum type="real" doc="Dry aerosol particles diameter [m]">2.6e-08</dgnum> | ||
<dgnumwet type="real" doc="Wet aerosol particles diameter [m]">0.41417721820867320E-007</dgnumwet> | ||
<wetdens type="real" doc="Wet density of interstitial aerosol [kg/m3]">0.15100083211582764E+004</wetdens> | ||
<dgnum type="array(real)" doc="Dry aerosol particles diameter [m]">1.37146e-07 ,3.45899e-08 ,1.00000e-06 ,9.99601e-08</dgnum> |
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 these initial values set for the 3D arrays? I suppose they will be modified later?
// Layout for 3D (2d horiz X 1d vertical) variable defined at mid-level and | ||
// interfaces | ||
const FieldLayout scalar3d_mid = grid_->get_3d_scalar_layout(true); | ||
const FieldLayout scalar3d_int = grid_->get_3d_scalar_layout(false); |
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.
Does "(true) or (false)" here mean whether this field is defined at layer center?
config_.amicphys.gaexch_h2so4_uptake_optaa = 2; | ||
config_.amicphys.newnuc_h2so4_conc_optaa = 2; | ||
using namespace ekat::units; | ||
constexpr auto q_unit = kg / kg; // units of mass mixing ratios of tracers |
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.
Can you confirm the unit for mixing ratios of trace gases (mol/mol or kg/kg)?
grid_->get_3d_vector_layout(true, nmodes, "nmodes"); | ||
|
||
// Geometric mean dry diameter for number distribution [m] | ||
add_field<Required>("dgnum", scalar3d_mid_nmodes, m, grid_name); |
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 "m" here looks the same as the mode index "m" at line 191? Could it cause any problem?
components/eamxx/src/physics/mam/eamxx_mam_microphysics_process_interface.cpp
Show resolved
Hide resolved
std::string MAMMicrophysics::name() const { | ||
return "mam4_micro"; | ||
} | ||
void MAMMicrophysics::set_namelist_params_() { |
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.
Will the parameters be correctly set for all processors? In the Fortran code we need to broadcast them.
// these parameters guide the coupling between parameterizations | ||
// NOTE: mam4xx was ported with these parameters fixed, so it's probably not | ||
// NOTE: safe to change these without code modifications. | ||
config_.amicphys.gaexch_h2so4_uptake_optaa = 2; |
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.
It would be nice to include the original description for these parameters.
Testing
We have included one SCREAM standalone test. Note that the baseline tests for these may fail because baselines are only created once the PR is merged.
mam4_aero_microphys
process is added as an EAMxx process with the ability to turn it on/off using the labelmam4_aero_microphys
in the namelist_scream.xml file. The atmchange command to invoke this process in a CIME simulation is:./atmchange physics::atm_procs_list="mac_aero_mic,rrtmgp,mam4_aero_microphys"
To run a CIME simulation, we have created a test modifier
mam4xx-aero_microphysics
:cd cime/scripts
Where,
is the machine name
is the compiler to use on the machine
is a unique string test identifier
is the allocation project to charge
For
ne30pg2
grid, use the following command:New input data
We have added 9 elevated emission files for each of the three grid resolutions:
ne2np4, ne4pg2, ne30pg2
For finer resolutions, the code will interpolate the ne30pg2 grid.
We have also added files for LINOZ, OXID, Chlorine, and photolysis.
All the files are on the E3SM input data server.
Standalone Test Timings (Compy):
(We will add the timings once the PR is approved)
Important notes: