First iteration of adding water tracer infrastructure#78
First iteration of adding water tracer infrastructure#78billsacks merged 11 commits intoESCOMP:mainfrom
Conversation
Move some shr_string unit tests to keep all of the list-related unit tests together.
|
@nusbaume and @mvertens - I'm adding both of you as reviewers. I think a review from one of you is sufficient. @nusbaume - I at least want you to be aware of this and look through it at some point, but I don't know how much you're working over the holidays... if you're not working much, then your review can wait until after the holidays... I might merge it in the meantime if I get a review from @mvertens but I'll be happy to make any changes in a follow-up PR. |
|
Thank you for the approval, @jedwards4b . I have done some cleanup suggested by @mvertens - in 3881bee @mvertens - I'll wait to merge this until you give your approval. Note that I decided not to do the pre-setting of rc = ios (or similar), because I felt that the extra information printed would have been unhelpful (and at least as confusing as helpful because there's no context to help you interpret an arbitrary error code integer). |
mvertens
left a comment
mvertens
left a comment
There was a problem hiding this comment.
@billsacks - I have been thinking that specifying mainproc at the module level as part of the shr_waters_init would be better than having to pass in as an argument to the routines. That way if you decide you want to add a new log message on mainproc for a routine that does not have mainproc as an argument - you can easily do that. I've taken that approach in my CDEPS refactor.
This way it can be used later without always needing to pass it as an argument.
Great idea - thanks, @mvertens . I have made that change. |
|
By the way, I wanted to call your reviewer attention to one aspect of this new module that is somewhat non-intuitive. As I document at the top of the module: ! Note that the init routine uses ESMF-style error handling (where a return code is
! returned up the call stack), whereas other routines abort directly if an error is
! detected. (The rationale for this is that the init routine has a lot of interaction
! with ESMF and is expected to be called from an ESMF-centric part of the code, whereas
! the other routines are less ESMF-centric in both their implementation and where they
! are expected to be called from, and so it doesn't make as much sense for them to
! follow the ESMF error handling paradigm).Please let me know if you have any concerns about this asymmetry. |
|
@billsacks - now I'm starting to wonder about my suggestion regarding mainproc. This will work fine if you have all the data model components sharing the same root processor. But what if you have two data components (e.g. DATM, DROF) running on mutually exclusive processors - or only sharing a subset of the processors - then mainproc is no longer uniquely defined. The same would go for using a module level logunit. |
Ah, very good point. I'll back out that most recent commit. |
This reverts commit b441fa6. As Mariana pointed out: "now I'm starting to wonder about my suggestion regarding mainproc. This will work fine if you have all the data model components sharing the same root processor. But what if you have two data components (e.g. DATM, DROF) running on mutually exclusive processors - or only sharing a subset of the processors - then mainproc is no longer uniquely defined. The same would go for using a module level logunit."
Refactor med_methods_FB_init to allow handling of water tracer fields ### Description of changes Rather than having med_methods_FB_init accept one of fieldNameList or STflds, now it accepts an array of med_field_info_type objects. med_field_info_type is defined in a new module that also includes some creation methods, making calls to med_methods_FB_init a two-step process: first create field_info_array, then call med_methods_FB_init with this field_info_array. A key new feature is that the creation of a field_info_array from field_names will assume a single ungridded dimension with size given by shr_wtracers_get_num_tracers for any field with the water tracer suffix. This was the main motivation for the refactor here. Some specific notes about the changes in this commit: - Behavior change: if there are only scalar or blank fields, we do some unnecessary work now (retrieving the mesh that we won't end up ever using) - I think I got the gridToFieldMap right: before it was hard-coded to 2, but I think that was only right for a single ungridded dimension; here I'm generalizing that for multiple ungridded dimensions **This depends on the CESM_share changes in ESCOMP/CESM_share#78 and ESCOMP/CESM_share#80 ### Specific notes Contributors other than yourself, if any: CMEPS Issues Fixed (include github issue #): Are changes expected to change answers? (specify if bfb, different at roundoff, more substantial) No - bfb Any User Interface Changes (namelist or namelist defaults changes)? No ### Testing performed Please describe the tests along with the target model and machine(s) If possible, please also added hashes that were used in the testing Ran a subset of the CESM prealpha tests with baseline comparisons, using cesm3_0_alpha08a with this CMEPS branch and the share branch from ESCOMP/CESM_share#78: ``` ERS_D_Ld3.ne30pg3_t232.B1850C_LTso.derecho_intel.allactive-defaultio ERI.ne30pg3_t232.B1850C_LTso.derecho_intel.allactive-defaultio ERC_D_Ln9.ne30pg3_ne30pg3_mt232.FHISTC_LTso.derecho_intel.cam-outfrq9s ERS_Ly7.f09_g17_gris4.T1850Gg.derecho_intel SMS_Lm13.f10_f10_mg37.I1850Clm50SpG.derecho_intel MULTINOAIS_Ly2.f10_f10_ais8gris4_mg37.I1850Clm50SpRsGag.derecho_intel.cism-change_params SMS_D_Ld1.ne30pg3_t232.I1850Clm50BgcSpinup.derecho_intel.clm-cplhist ERI.TL319_t232.G_JRA.derecho_intel.cice-default SMS_D.TL319_t232.G_JRA_RYF.derecho_intel SMS_Ld40.TL319_t232.C_JRA.derecho_intel ``` All tests pass and are bit-for-bit with the baselines. There were NLCOMP failures in some tests; from spot-checking, the NLCOMP failures all seem to be in MOM files. Nothing in this PR or the accompanying share PR could change the namelists, so I assume these are a pre-existing issue: ``` FAIL ERI.ne30pg3_t232.B1850C_LTso.derecho_intel.allactive-defaultio NLCOMP FAIL ERI.TL319_t232.G_JRA.derecho_intel.cice-default NLCOMP FAIL ERS_D_Ld3.ne30pg3_t232.B1850C_LTso.derecho_intel.allactive-defaultio NLCOMP FAIL SMS_D.TL319_t232.G_JRA_RYF.derecho_intel NLCOMP FAIL SMS_Ld40.TL319_t232.C_JRA.derecho_intel NLCOMP ```
3 participants
This PR adds
shr_wtracers_modto coordinate adding arbitrary water tracers across CESM. This also addsshr_string_withoutSuffix, which can be used to determine if a given field is a water tracer field, and if so, return the name of the field without the water tracer suffix.I'll be adding more functionality in a follow-up PR, but some of the changes here are needed for a CMEPS PR that I'm about to open.