-
Notifications
You must be signed in to change notification settings - Fork 79
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
CMEPS PR for CDEPS Inline implementation #420
Conversation
* add flag to track lake freezing for clm lake
@uturuncoglu I think a merge of main into your branch should fix the github testing. |
@jedwards4b Okay. I'll do it. I think I am referencing their CDEPS for for this but once it is ready to merge, I'll sync with master. NOAA fork has some data modes that are not pushed to ESCOMP. Also, I have no information about the process of creating input files (incl. meshes) for thoıse data modes. We could push them but they don't have any entry in the CIME interface. Do you think we could still push them to ESCOMP? |
Do you suppose maybe they pushed to a NOAA fork and not to the main repo because they wanted to avoid our rigorous review and testing process??? |
@jedwards4b Not sure. We need to coordinate with them. Maybe I could create a branch (forked from ESCOMP) that will only have changes related to CMEPS+CDEPS inline capability. This will allow us to get that feature to CESM but not the extra data modes which seems reasonable solution. |
Yes, I like that idea. Thanks |
@jedwards4b I would like to test this with CESM but I am not sure it can be done on Derecho. Please let me know which tests needs to be run and with which baseline. Then, I could perform some initial test. BTW, i am seeing following error in CI script regression tests but not sure why,
I have some changes in the CDEPS side to related with |
@uturuncoglu I don't see any PR with changes to dshr_stream_mod.F90 in CDEPS and there is no variable src_mask_val in the derived type. |
Yes, I think that you need to bring in cdeps changes first. |
@jedwards4b I think this is ready for review. I still need to test it with CESM and add some fix for UFS Weather Model field exchange file (the UFS one) but UFS RT are fine. I'll also test ATM+OCN+WAV configuration with MOM6 and that could require some minimal change. This is also linked with ESCOMP/CDEPS#259. |
@jedwards4b Please also note that I also did not modify the CMEPS core build system to activate the CDEPS inline. Also, we need to make some changes in the CIME interface to support new options. |
@uturuncoglu I tested your latest commit on this branch for all UWM non-standalone ATM tests and it passed. I only needed to merge in the latest emc/develop to get your changes for the land component. Just to note---I had tested the total/select change previously, but only for a single test. It turns out that it failed in several tests, but not the one I used. |
@DeniseWorthen Okay that is great. It seems that we don't have issue in UFS side. I will also update the branch with land changes. I still need to test with CESM. I'll do it today and let you know. Thanks for your help. |
@jedwards4b scripts regression tests are still failing because of the newly added namelist options in CDEPS side. I check the action and it seems it's getting CDEPS from CESM repository and since CESM is not using lates version it is failing. We might need to update workflow or CESM to make it work again. I also start testing the fix and I'll compare with the baseline. Keep you updated. |
@DeniseWorthen @jedwards4b JFYI, commit ea995f6 basically brings recent changes made for the land component coupling under UFS. |
@jedwards4b I am creating baseline with following command,
and checking agains with following,
The issue is that the second command tries to check the baseline with |
@jedwards4b I might find the issue. I am trying to create baselines again. Maybe I created the baseline with different version of CIME. Anyway testing now. |
@jedwards4b Okay. The naming issue seems solved but I am still getting error like following,
The directory over there but I have also fix the issue when add_gusts is turned on. The @DeniseWorthen @BinLiu-NOAA @binli2337 Are you fine with the current state of the CMEPS? I think we were fine in the UFS side (we need to check last add_gusts fix in UFS end too) but you might still need something else in UFS side. Anyway, I just want to double check. If everything is fine we could merge this PR. @jedwards4b you might want to review this again since there are some changes like fixing the issue in CESM side. You might want to double check if you want. Here is the script that shows the results - |
@jedwards4b To be sure I also run cmeps prealpha tests again. I have some failures like following,
In this case
and
You could see the failed 3 tests using |
@DeniseWorthen @BinLiu-NOAA I just wonder if you need any additional change in this PR. Are all tests fine with this version. @jedwards4b please let me know if you need more testing. If everybody are fine with it then I think we could ready for merge and after that I could create another CMEPS PR to update NOAA-EMC fork. |
@uturuncoglu I wasn't sure how your testing at ESCOMP was going. If everything looks good on that end, I think it's ready to merge. Then yes, please create a PR back to NOAA-EMC, and add that PR to your inline-cdeps PR for UWM. |
@DeniseWorthen It seems that CESM tests works fine. @jedwards4b Could you confirm it. Then I could go ahead and merge this 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.
Looks good Ufuk - just a few changes requested.
@jedwards4b Thanks. Do you want me to merge it. Or waiting for the test to finish. Probably it will fail unless you update the cdeps version in the CESM side. |
@jedwards4b JFYI, set test failed again with cprnc issue. |
@jedwards4b It seems that there is an issue in checking out https://github.com/ESMCI/cprnc. The srt test has issue with accessing submodule under https://github.com/ESMCI/cime/tree/master/CIME/non_py |
right, working on it now |
Description of changes
This PR aims to bring CDEPS inline capability to CMEPS to fill unmapped regions for the regional modeling configurations. More information can be found in the following presentation: https://docs.google.com/presentation/d/1Tk76zlsRiT7_KMJiZsEJHNvlHciZJJBBhsJurRMxVy4/edit#slide=id.g2613ed2f8f8_0_0
This PR is also linked to complete the implementation: ESCOMP/CDEPS#259
Specific notes
Contributors other than yourself, if any: @danrosen25 @binli2337 @BinLiu-NOAA
CMEPS Issues Fixed (include github issue #): No
Are changes expected to change answers? (specify if bfb, different at roundoff, more substantial) No
Any User Interface Changes (namelist or namelist defaults changes)? No.
The user need to add new phase (
med_phases_cdeps_run
) to the run sequence to activate the feature. Then, needs to providestream.config
configuration file.Additional changes also made to allow filling exchange fields with all data in the first coupling time step. I introduce set of new mediator namelist option to make it configurable. So, for example I am setting following in the nems.configure for ocean and wave (in mediator section),
By default the values are .false. and it is only usable when CDEPS Inline feature activated. I also modify the wave and ocean prep phases to get the data and pass it to the components.
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
This is initially tested under UFS Weather Model and did. not change the answer for
hafs_regional_atm_ocn_wav
andcpld_control_p8
cases. More test will be performed with both UFS Weather Model and also CESM.