Adds capability to read fractional land use file in MAM4 dry deposition #3050
Conversation
singhbalwinder
requested review from
bartgol,
mahf708,
susburrows,
odiazib and
AaronDonahue
|
@susburrows: I have chosen the reviewers for this PR. I will add more info to the PR message while addressing the reviewers' comments. |
| <!-- MAM4xx-Surface-Emissions --> | ||
| <mam4_srf_online_emiss inherit="atm_proc_base"> | ||
|
|
||
| <srf_emis_specifier_for_DMS hgrid="ne4np4.pg2" type="file" doc="File containing surface emissions data for DMS">${DIN_LOC_ROOT}/atm/scream/mam4xx/emissions/ne4pg2/surface/DMSflux.2010.ne4pg2_conserv.POPmonthlyClimFromACES4BGC_c20240814.nc</srf_emis_specifier_for_DMS> |
There was a problem hiding this comment.
This block is not removed. It is just moved below so that the xml parser can choose the right files for the specified resolution. The parser expects the files in a particular order.
There was a problem hiding this comment.
Looks good to me but note I am not super familiar with the IO done in spa (more familiar with the nudging stuff). So I will defer to Aaron and Luca for a more definitive review
Seems like you just need to read frac_landuse_.data.frac_land_use from the file once (time independent, etc.), right? If I understood the code correctly, I would personally opt to just retrieving that alone from the second function you added, update_frac_land_use_data_from_file, without transporting the reader and interpolator objects back and forth (so no outs in the first function, init_frac_landuse_file_read); maybe you have other plans for utilizing these for more action later on, though even then, those could be added later (so no need to add them now). But up to you, just a minor recommendation
| static void update_frac_land_use_data_from_file( | ||
| std::shared_ptr<AtmosphereInput> &scorpio_reader, | ||
| AbstractRemapper &horiz_interp, FracLandUseInput &input); | ||
|
|
||
| // ------------------------------------------------------------------------------------------- | ||
| // ------------------------------------------------------------------------------------------- | ||
|
|
||
| static void init_frac_landuse_file_read( |
There was a problem hiding this comment.
consider adding unit tests for both of these functions to ensure they are robust
There was a problem hiding this comment.
Could you please guide me on how to add a unit test for this function? We have a single process test, and these functions are called part of that test.
There was a problem hiding this comment.
I meant something like components/eamxx/src/physics/spa/tests/spa_read_data_from_file_test.cpp
Because the process test you have already tests these two functions (because it runs everything), this addition is NOT a must for the current PR; it could be an assignment for a software-minded person on the team down the line
components/eamxx/src/physics/mam/eamxx_mam_dry_deposition_process_interface.cpp
Outdated
Show resolved
Hide resolved
components/eamxx/src/physics/mam/eamxx_mam_dry_deposition_process_interface.cpp
Outdated
Show resolved
Hide resolved
|
Thanks, @mahf708 for the quick review. I like the idea of making just one call to retrieve the data but I also see value in following the original design:
|
|
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: singhbalwinder |
|
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 # 6184 FAILED (click to see last 100 lines of console output)SCREAM_PullRequest_Autotester_Mappy # 5936 FAILED (click to see last 100 lines of console output) |
…files from testmods
…y to work array space
singhbalwinder
force-pushed
the
singhbalwinder/read-frac-land-use-file
branch
from
51d9309 to
0b91276
Compare
|
Correction on the This test has a baseline, but the baseline is only generated for the initial time step (I am recording these details here so that I remember the manual tests I ran to ensure the code changes are producing the expected results) |
|
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. |
I don't think this is the desired behavior. Maybe there's an issue in the test specs in the cmake file. I'll check. |
|
Ah, yes, the baseline test logic is wrong. It assumes that there is only one file to check, perhaps b/c it assumes we put all snaps in the same file. I will change the max snaps per file in the output specs, so that this single file is enough. |
|
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: singhbalwinder |
|
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 # 6233 FAILED (click to see last 100 lines of console output)SCREAM_PullRequest_Autotester_Mappy # 5973 FAILED (click to see last 100 lines of console output) |
|
@singhbalwinder confirmed the fails are expected. Merging. |
The capability of reading fractional land use is added to replace the hardwired values
in dry deposition. This file requires only horizontal interpolation. There is no time
dimension, so time interpolation is not needed. All the data is read and interpolated
during the init phase of the simulation. This PR also brings a few other additions:
ntracersthat is used to choose the init file used for the model for various resolutions. This change allowed us to remove a line that sets init file in the test mod files of all MAM4xx tests. This also enabled MAM4xx CIME simulations to be easy to run for all the resolutions./theta-l/share/prim_state_mod.F90fileInput data
Input data has been uploaded to input data server:
/lcrc/group/acme/public_html/inputdata/atm/scream/mam4xx/drydep