Sync generic working in UFS#17
Conversation
update datm/datm_datamode_gfs_mod.F90
Update CDEPS to include the latest changes from ESCOMP/CDEPS
Update CDEPS to include the recent changes from ESCOMP/CDEPS
Update NOAA-EMC/CDEPS to include changes from ESCOMP/CDEPS.
Update cdeps to include the latest changes from ESCOMP/CDEPS
* Adapt docn_datamode_cplhist for dice_datamode_cplhist * Fix .or. and local variables have different names in docn and dice * Add Si_imask. Decide whether to have flds_i2o_per_cat for cplhist method. * Copy sea ice stream fields to export state fields via dshr_dfield_add * Switches So_t from C to K. Decide on which/add switch * Overwrite missing value of 0 K in Si_t * Handle So_t in C and K in docn_datamode_cplhist_mod.F90 Need model SST to be in K if (minval(So_t) .LT. 100.0_r8), convert C-->K * Only fill docn_datamode_cplhist export variables if associated. Handle units of So_t based on namelist input * Default convert SST C to K (as currently). Then switch based on optional sst_constant_value * Allow null return for taux too in dice/dice_datamode_cplhist_mod.F90 * Add doc for dice cplhist mode to dice.rst
* For gfs and cfsr datm datamodes, use gefs * Delete datm/datm_datamode_gfs_hafs_mod.F90 * Remove gfs_hafs datamode from atm_comp_nuopc.F90 * Remove gfs_hafs from CMakeLists.txt
Add generic datamode to sync to ESCOMP in test branch
rc not optional in datamode_generic_init_pointers
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request removes the fox and genf90 submodules, adding them to .gitignore, and refactors the initialization of shared stream data (sdat) across multiple components by passing it to dshr_init instead of dshr_mesh_init. It also adds several new export fields to the ERA5 data mode in datm and updates the generic advertising logic in dshr_generic_mod.F90. Feedback on these changes identifies a potential runtime crash due to an optional argument rc being passed to ChkErr without a presence check, a duplicated statement in docn, unused variables in dice, and a hardcoded component-specific log message in a shared utility module.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| integer, intent(out), optional :: rc | ||
|
|
||
| integer :: i, n | ||
| character(len=CL) :: fieldName | ||
| type(fldlist_type), pointer :: fldList | ||
|
|
||
| if (present(rc)) rc = ESMF_SUCCESS |
There was a problem hiding this comment.
In datamode_generic_advertise, rc is declared as an optional argument. However, it is passed directly to ChkErr(rc, ...) on line 71. If rc is not present, passing it to a non-optional dummy argument in ChkErr will cause undefined behavior or a runtime crash. To resolve this, either make rc non-optional (consistent with datamode_generic_init_pointers) or use a local status variable to capture the return code and check it.
integer, intent(out) :: rc
integer :: i, n
character(len=CL) :: fieldName
type(fldlist_type), pointer :: fldList
rc = ESMF_SUCCESS
| rtmp(1) = sst_constant_value | ||
| if(export_all) bcasttmp(4) = 1 |
There was a problem hiding this comment.
The statement if(export_all) bcasttmp(4) = 1 is duplicated on line 263, as it was already executed on line 261. This redundant statement should be removed.
rtmp(1) = sst_constant_value
| integer :: bcasttmp(4) | ||
| real(r8) :: rbcasttmp(3) | ||
| type(ESMF_VM) :: vm | ||
| logical :: isPresent, isSet |
There was a problem hiding this comment.
The variables isPresent and isSet are declared in InitializeAdvertise but never used. They should be removed to keep the code clean.
| do while (associated(fldlist)) | ||
| call NUOPC_Advertise(exportState, standardName=fldlist%stdname, rc=rc) | ||
| if (ChkErr(rc,__LINE__,u_FILE_u)) return | ||
| call ESMF_LogWrite('(datm_comp_advertise): Fr_atm'//trim(fldList%stdname), ESMF_LOGMSG_INFO) |
There was a problem hiding this comment.
The log message hardcodes (datm_comp_advertise): Fr_atm, which is specific to the DATM component. Since dshr_generic_mod.F90 is a shared utility module, hardcoding component-specific strings reduces its reusability and can be misleading if used by other components. Consider using a generic prefix or passing the component name as an argument.
call ESMF_LogWrite('(dshr_generic_advertise): '//trim(fldList%stdname), ESMF_LOGMSG_INFO)
5 participants
Description of changes
Specific notes
Contributors other than yourself, if any:
CDEPS Issues Fixed (include github issue #):
Are there dependencies on other component PRs (if so list):
Are changes expected to change answers (bfb, different to roundoff, more substantial):
Any User Interface Changes (namelist or namelist defaults changes):
Testing performed (e.g. aux_cdeps, CESM prealpha, etc):
Hashes used for testing: