Skip to content
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

sync CDEPS with ESCOMP #60

Merged
merged 68 commits into from
Feb 5, 2024
Merged

Conversation

uturuncoglu
Copy link
Collaborator

Description of changes

This PR aims to sync CDEPS NOAA-EMC fork with ESCOM to bring support for configurable source and destination mask, which required for regional HAFS configuration.

Specific notes

Contributors other than yourself, if any: @binli2337 @BinLiu-NOAA @DeniseWorthen

CDEPS Issues Fixed (include github issue #): N/A

Are there dependencies on other component PRs (if so list): No

Are changes expected to change answers (bfb, different to roundoff, more substantial): No

Any User Interface Changes (namelist or namelist defaults changes):
The two new optional namelist option is introduced to the ESMF config file: stream_src_mask and stream_dst_mask to control mask values. The deafest are zero which does not change the existing behavior. Additional code needs to be implemented if we want to support XML configuration files.

Testing performed (e.g. aux_cdeps, CESM prealpha, etc): Using both CESM and UFS

Hashes used for testing:

alperaltuntas and others added 30 commits December 18, 2022 18:54
in this initial commit, it is just a copy of CORE2-NYF.
Update stream definitions for new coupler history file format

### Description of changes

Modify stream_definition_datm.xml to generate a streams file (datm.streams.xml) with the new coupler history file format.

### Specific notes

Changes to accommodate new coupler history file names.
Change offset for solar stream from 2700 to -900 to accommodate changes due to time stamps.
These changes work in conjunction with CDEPS PR ESCOMP#224 and CDEPS PR ESCOMP#222 .
Note that I did not change the file names for ndep, or remove that stream. See ESCOMP#230

Contributors other than yourself, if any: @billsacks 

CDEPS Issues Fixed (include github issue #):  N/A

Are there dependencies on other component PRs (if so list):  No

Are changes expected to change answers (bfb, different to roundoff, more substantial):  Yes, in coupler history mode.

Any User Interface Changes (namelist or namelist defaults changes): No

Testing performed (e.g. aux_cdeps, CESM prealpha, etc):  I have conducted a pair of cases, an F-case to generate coupler history files, and an I-case to read those files, using the new file name convention, and compared the forcing output variables from clm history files between the two cases.  @billsacks and I reviewed these differences and found them to be acceptable.

@billsacks ran SMS_D_Ld1.ne30pg3_t061.I1850Clm50BgcSpinup.cheyenne_intel.clm-cplhist in the context of ESCOMP/CTSM#1999

Hashes used for testing:  N/A
update github to make cdeps ext build an action
Update SST files for historical configurations
Enable build with ESMX and introduce export_all
uturuncoglu and others added 2 commits January 26, 2024 15:49
update default SSTICE_DATA_FILENAME to match that previously used in cam
@binli2337 binli2337 self-requested a review January 28, 2024 10:33
Copy link
Collaborator

@binli2337 binli2337 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. Approved.

@DeniseWorthen
Copy link

@uturuncoglu If I compare this branch to ESCOMP/main, there are still changes related to the inline implementation (hafs data mode). So this PR brings in the latest ESCOMP + your inline changes to EMC. But another PR back to ESCOMP will be needed to add the inline feature at ESCOMP/CDEPS, right?

@uturuncoglu
Copy link
Collaborator Author

@DeniseWorthen In terms of changes related to CDEPS, they are already in ESCOMP. For CMEPS, we first need to merge CMEPS PR on ESCOMP and then there will be another PR in EMC side to bring those to NOAA-EMC CMEPS fork. I did not create that PR yet but if you want I could create. Of course that needs to be merged after we merge the CMEPS PR in ESCOMP side. I hope my answer is clear. Let me know if it is not and want me to create PR in NOAA CMEPS fork.

@DeniseWorthen
Copy link

Hm, if I look here
ESCOMP/CDEPS@main...uturuncoglu:CDEPS:feature/mask

There are changes to 4 files that your branch has relative to ESCOMP?

@uturuncoglu
Copy link
Collaborator Author

@DeniseWorthen okay. Let me double check.

@uturuncoglu
Copy link
Collaborator Author

@DeniseWorthen i think it normal to see these difference since you are comparing with ESCOMP and it does not have those data modes developed by NOAA. If you really want to push those to CDEPS ESCOMP we need to modify the cime interface to handle those under CESM. I did it when i introduce the ERA5 mode but not for others since I did not develop those. Anyway, I think we are fine.

@DeniseWorthen
Copy link

Ok, thanks. I understand.

@BinLiu-NOAA
Copy link

@uturuncoglu, @DeniseWorthen ,and @binli2337, it looks to me the PR branch: https://github.com/uturuncoglu/CDEPS/tree/feature/mask has not yet synced with the target branch: https://github.com/NOAA-EMC/CDEPS/tree/develop
I thought that is required before merging the PR, is this true?

@DeniseWorthen
Copy link

@BinLiu-NOAA We do not update subcomponents until we commit the update in UWM. So each UWM PR should pull from a feature branch for a subcomponent. Then we test in UWM and only when testing is complete we update the top of the subcomponents.

@BinLiu-NOAA
Copy link

BinLiu-NOAA commented Feb 1, 2024

@BinLiu-NOAA We do not update subcomponents until we commit the update in UWM. So each UWM PR should pull from a feature branch for a subcomponent. Then we test in UWM and only when testing is complete we update the top of the subcomponents.

@DeniseWorthen, I understand.

The issue is @uturuncoglu 's feature/mask branch did not sync with his last set of changes committed into NOAA-EMC/CDEPS through PR #59. Here are differences (after syncing and solving conflicts by removing the duplicated code sections related to case('SIMPLE'): in datm/atm_comp_nuopc.F90):

uturuncoglu/CDEPS@feature/mask...hafs-community:CDEPS:feature/mask

With the new updated hafs-community:CDEPS:feature/mask branch, @binli2337 was able to run the failed atm_cdeps_lnd_era5 RT now on Hera (after adding "stream_vectors01: null" in tests/parm/datm.streams.era5.IN).

With that, I created the following PR (uturuncoglu#1) into @uturuncoglu 's feature/mask branch. After he accepts this PR, then it should be good to go (Plus, I will update tests/parm/datm.streams.era5.IN accordingly as well).

Cheers!

@DeniseWorthen
Copy link

@BinLiu-NOAA I understand now. That's great you figured it out.

@BinLiu-NOAA
Copy link

@BinLiu-NOAA I understand now. That's great you figured it out.

Thanks @DeniseWorthen! Glad it works now. Thanks for @binli2337 for all the discussions and testing!

@jkbk2004
Copy link
Collaborator

jkbk2004 commented Feb 5, 2024

All tests are done at ufs-community/ufs-weather-model#2028 @binli2337 can you merge this pr?

@binli2337 binli2337 merged commit 3d7067a into NOAA-EMC:develop Feb 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.