search

ESCOMP / CDEPS

Community Data Models for Earth Prediction Systems
https://escomp.github.io/CDEPS/versions/master/html/index.html
18 stars 39 forks source link

Add nuopc shr methods #269

Open jedwards4b opened 3 months ago

jedwards4b commented 3 months ago

Description of changes

move nuopc_shr_methods.F90 from cmeps/cesm/nuopc_cap_share

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:

jedwards4b commented 3 months ago

This moves the file nuopc_shr_methods.F90 from cmeps/cesm/nuopc_cap_share to cdeps/share with history so that it can be used by ufs. Note that the functionality here is not supported by ufs and so an error will be generated, but it will compile.

uturuncoglu commented 3 months ago

@jedwards4b Do you want me to test this with UFS? or wait you to fix any outstanding issue.

jedwards4b commented 3 months ago

You can test this with UFS - I am working on an issue with REST_OPTION=end but that can be separate from this initial test of functionality.

uturuncoglu commented 3 months ago

@jedwards4b I run couple of data component test with UFS Weather Model develop and all pass without any issue. Here is the list of tests:

datm_cdeps_mx025_cfsr - intel
datm_cdeps_lnd_era5_rst - intel
datm_cdeps_restart_cfsr - intel

I think this PR is fine for UFS.

mvertens commented 2 months ago

@jedwards4b - is this ready to be merged?

jedwards4b commented 2 months ago

@mvertens I think that there is still an issue with building it for cesm and I wasn't planning to bring it in until after beta17.

mvertens commented 2 months ago

@jedwards4b - thanks for the update.

jedwards4b commented 3 weeks ago

@uturuncoglu I would prefer to remove the share code from cdeps completely and require ufs to use https://github.com/ESCOMP/CESM_share. If including another repository is an issue then maybe a fork or ufs branch could include it without it being in the cesm development tree.

uturuncoglu commented 3 weeks ago

@jedwards4b It is fine for me but I am not sure about what UFS developer team think. So, it would be nice to inform them about this. @junwang-noaa @DeniseWorthen maybe we could discuss it in the next infrastructure meeting. Let me know what you think.

NickSzapiro-NOAA commented 3 weeks ago

Hi. I'm a code manager for https://github.com/NOAA-EMC/CDEPS/ for UFS. Adding a submodule is not without overhead. It would be very nice to discuss this further, especially if there are shared benefits to these changes. If I may ask, does CESM_share introduce any additional dependencies/requirements?

jedwards4b commented 3 weeks ago

@NickSzapiro-NOAA No CESM_share does not add any additional dependencies. It did in the past and that I think is why we needed to make a copy of it without them, but those have been removed. I don't like having multiple copies of the same source around and so now that ufs cdeps can use the cesm_share repo instead of having it's own copy I would really like to delete the copy.

DeniseWorthen commented 3 weeks ago

In ufs, I see the use of nuopc_shr_methods in CMEPS-interface/CMEPS/cesm as well as in the following locations:

CDEPS-interface/CDEPS/dshr/dshr_mod.F90:117:    use nuopc_shr_methods, only : set_component_logging
CMEPS-interface/CMEPS/mediator/med.F90:568:    use nuopc_shr_methods, only : set_component_logging
MOM6-interface/MOM6/config_src/drivers/nuopc_cap/mom_cap.F90:36:use nuopc_shr_methods, only: get_component_instance

In each of these cases, both the use nuopc_shr_methods as well as the associated calls are w/in a CESMCOUPLED ifdef block.

The removal of the nuopc_shr_methods from CMEPS/cesm should have no impact on UFS.

As far as removing all of CDEPS/share from CDEPS itself, we do use that code so I think that needs a larger discussion.

jedwards4b commented 3 weeks ago

@DeniseWorthen The code is in https://github.com/ESCOMP/CESM_share there is no reason to duplicate it in cdeps and duplicating code is error prone.

DeniseWorthen commented 3 weeks ago

I understand why you want to do it, but for UFS it requires adding another submodule and maintaining it.

jedwards4b commented 3 weeks ago

You could alternatively create a fork of cdeps with these files and maintain that. The way it is now is requiring me to maintain two repositories of these shared files.

DeniseWorthen commented 3 weeks ago

We already maintain a fork of CDEPS. I can see that moving these files gives you one less thing to manage. We'd then be on the hook for ensuring that we keep up to date w/ any changes that might come in to CESM_share. I see a lot of commits to CESM_share, but it's unclear to me how much is actually changing w/ those commits.

Also, I see in CESM_share some of the F90.in files that required the use of the FOX library (?), which we couldn't do because of operational platforms. Isn't that going to be a conflict if we were to use CESM_share? (I also don't know if CESM_share has other dependencies like FOX which would be a problem.)

jedwards4b commented 3 weeks ago

The .F90.in files require the use of genf90.pl something I think you already use.

DeniseWorthen commented 3 weeks ago

We actually use that outside of the repo and update the files manually. We can't use genf90.pl directly because of operational restrictions.

jedwards4b commented 3 weeks ago

Most of those recent commits in CESM_share were debugging the github workflow and didn't actually change any source. It sounds like you should consider maintaining a copy of these files on your fork.