Add CDEPS component, add CDEPS tests and fix bugs in the datm_bulk_gefs test#471
Add CDEPS component, add CDEPS tests and fix bugs in the datm_bulk_gefs test#471binli2337 wants to merge 20 commits into
Conversation
* Added 8 new cdeps tests.
aerorahul
left a comment
aerorahul
left a comment
There was a problem hiding this comment.
I don't think one needs two flags at the moment. It is understood that if CDEPS is ON, then it is DATM.
There was a problem hiding this comment.
Please add 1 deg. tests. Not 1/4 deg.
They were supposed to be added when DATM was added, but was never followed through.
@aerorahul There are six 1-degree datm tests in the current develop branch:
datm_control_cfsr
datm_control_gefs
datm_bulk_cfsr
datm_bulk_gefs
datm_restart_cfsr
datm_debug_cfsr
|
I don't understand why there needs to be both a @DeniseWorthen The two files have been merged. |
binli2337
added
enhancement
* Added datm tests. * Updated scripts used for the data-atmosphere tests. * Fixed bugs in the datm_bulk_gefs test.
binli2337
changed the title
binli2337
added
the
Waiting for Reviews
|
Could CDEPS be built with a libcdeps.a where the atm, ocn and other data
components are included, and at run time we can choose which data component
will use, etc "datm_cfsr" or "datm_gefs"?
…On Thu, Mar 18, 2021 at 12:34 PM Dusan Jovic ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In CMakeLists.txt
<#471 (comment)>
:
> @@ -295,6 +310,12 @@ target_link_libraries(ufs PUBLIC esmf)
if(DATM)
target_link_libraries(ufs PUBLIC datatm)
+elseif(CDEPS_DATM)
+ list(APPEND _ufs_defs_private CDEPS
+ FRONT_CDEPS_DATM=atm_comp_nuopc)
+ add_dependencies(ufs datm)
+ include_directories(${CMAKE_CURRENT_BINARY_DIR}/CDEPS/datm)
Then CDEPS project should define multiple targets, one for each of the
data components, and each of those targets should define it's own include
directory and library (.a file). The application can then just specify
dependency on one (or more) of those data component targets. The
application should not directly depend on common/shared libraries. They
should be implicit dependencies. UFS should know nothing about those.
—
You are receiving this because your review was requested.
Reply to this email directly, view it on GitHub
<#471 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AI7D6TJYBZEMNMC2AWEHLL3TEITSXANCNFSM4ZCILWAQ>
.
|
|
I don’t think it is good idea to create single library file for entire data components supported by the CDEPS. This will increase the executable size since you will also include the support for all data component even they are not used by the model such as data land and data runoff etc. If you insist to go with this approach, you could copy the module files to a common location and you could combine the libraries externally (in UFS model level) in cmake build using ar function.
… On Mar 18, 2021, at 10:38 AM, Jun Wang ***@***.***> wrote:
Could CDEPS be built with a libcdeps.a where the atm, ocn and other data
components are included, and at run time we can choose which data component
will use, etc "datm_cfsr" or "datm_gefs"?
On Thu, Mar 18, 2021 at 12:34 PM Dusan Jovic ***@***.***>
wrote:
> ***@***.**** commented on this pull request.
> ------------------------------
>
> In CMakeLists.txt
> <#471 (comment)>
> :
>
> > @@ -295,6 +310,12 @@ target_link_libraries(ufs PUBLIC esmf)
>
> if(DATM)
> target_link_libraries(ufs PUBLIC datatm)
> +elseif(CDEPS_DATM)
> + list(APPEND _ufs_defs_private CDEPS
> + FRONT_CDEPS_DATM=atm_comp_nuopc)
> + add_dependencies(ufs datm)
> + include_directories(${CMAKE_CURRENT_BINARY_DIR}/CDEPS/datm)
>
> Then CDEPS project should define multiple targets, one for each of the
> data components, and each of those targets should define it's own include
> directory and library (.a file). The application can then just specify
> dependency on one (or more) of those data component targets. The
> application should not directly depend on common/shared libraries. They
> should be implicit dependencies. UFS should know nothing about those.
>
> —
> You are receiving this because your review was requested.
> Reply to this email directly, view it on GitHub
> <#471 (comment)>,
> or unsubscribe
> <https://github.com/notifications/unsubscribe-auth/AI7D6TJYBZEMNMC2AWEHLL3TEITSXANCNFSM4ZCILWAQ>
> .
>
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub <#471 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAJMBR55Y72FINMMBAK3LQDTEIUAXANCNFSM4ZCILWAQ>.
|
@junwang-noaa |
|
@aerorahul i think there is no need to create external interface for CDEPS since i have already updated its cmake build system to remove the dependency to have external build module and it works fine. I am not sure why do you need other layer at this point.
… On Mar 18, 2021, at 10:59 AM, Rahul Mahajan ***@***.***> wrote:
Could CDEPS be built with a libcdeps.a where the atm, ocn and other data components are included, and at run time we can choose which data component will use, etc "datm_cfsr" or "datm_gefs"?
… <x-msg://223/#>
On Thu, Mar 18, 2021 at 12:34 PM Dusan Jovic @.***> wrote: @DusanJovic-NOAA <https://github.com/DusanJovic-NOAA> commented on this pull request. ------------------------------ In CMakeLists.txt <#471 (comment) <#471 (comment)>> : > @@ -295,6 +310,12 @@ target_link_libraries(ufs PUBLIC esmf) if(DATM) target_link_libraries(ufs PUBLIC datatm) +elseif(CDEPS_DATM) + list(APPEND _ufs_defs_private CDEPS + FRONT_CDEPS_DATM=atm_comp_nuopc) + add_dependencies(ufs datm) + include_directories(${CMAKE_CURRENT_BINARY_DIR}/CDEPS/datm) Then CDEPS project should define multiple targets, one for each of the data components, and each of those targets should define it's own include directory and library (.a file). The application can then just specify dependency on one (or more) of those data component targets. The application should not directly depend on common/shared libraries. They should be implicit dependencies. UFS should know nothing about those. — You are receiving this because your review was requested. Reply to this email directly, view it on GitHub <#471 (comment) <#471 (comment)>>, or unsubscribe https://github.com/notifications/unsubscribe-auth/AI7D6TJYBZEMNMC2AWEHLL3TEITSXANCNFSM4ZCILWAQ <https://github.com/notifications/unsubscribe-auth/AI7D6TJYBZEMNMC2AWEHLL3TEITSXANCNFSM4ZCILWAQ> .
@junwang-noaa <https://github.com/junwang-noaa>
We should add a CDEPS-interface and include a CMakelists.txt there rather than relying on CDEPS to give us a library and a target.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub <#471 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAJMBR43B7B55VD36MQYDVTTEIWO7ANCNFSM4ZCILWAQ>.
|
Because it is not relevant to the UFS and does a lot of "unconventional" and "unsavoury" things. Just to spot a few. |
This is not how linkers work. Linker will not include the code for the components that are not used into the executable. The size of the library file (.a) has nothing to with the size of the executable. |
|
@DusanJovic-NOAA Okay. That is good to know. Then you can merge those libraries externally without any problem. |
No. We should not need to do any merging externally. That's not what we (UFS) should be doing. I think CDEPS should provide multiple targets, one for each of the components. Then in ufs, based on which type of application we are building, we can just define dependency on one or more of cdeps' targets, and that's all. No need to explicitly set include directories, no need to know anything about the fact that those targets internally depend on shared libraries etc. |
|
@DusanJovic-NOAA yes, i agree. i was taking about it when I said externally. sorry for confusion. |
What @DusanJovic-NOAA is suggesting is that there is a single |
|
@binli2337 I cannot find pio/2.5.2 on Gaea. Have you been able to test CDEPS there? @DeniseWorthen The pio/2.5.2 is not required for CDEPS. CDEPS is tested on Gaea with pio/2.5.1. |
|
there was a discussion happening here. Where are we with the suggestions in this PR? |
Update CMakeLists.txt to build the mom6_cice6_cdeps_datm application.
|
The mom6_cice6_cdeps_datm application can now be built by including the following lines in rt.conf: |
|
@binli2337 I just wonder the status of this PR. Any estimates about it? I am still waiting for this PR in HAFS application. |
|
@uturuncoglu This PR is on hold. The proposed changes include adding CDEPS-interface directory, adding a CMakeLists.txt file in CDEPS-interface, removing genf90 and FoX dependencies. |
|
This pull request is replaced by #538 |
*This PR addresses part 2 of CCPP issue ufs-community#748 to activate the exponential-random cloud overlap method (iovr=5) in RRTMG.
8 participants
Description
Issue(s) addressed
fixes: NOAA-EMC/NEMS#91
fixes: NOAA-EMC/CDEPS#6
fixes: NOAA-EMC/CMEPS#34
fixes: NOAA-EMC/CMEPS#37
fixes: #446
fixes: #456
Related PRs
NOAA-EMC/NEMS#92
NOAA-EMC/CDEPS#13
NOAA-EMC/CMEPS#36
Testing
Regression tests will be done on Hera, Orion, WCOSS-p3, WCOSS-Cray, Gaea, Jet and Cheyenne.
Timing information
(Walltime on Hera)
datm_cdeps_bulk_cfsr: 2.5 minutes for the 1-day run using 40 cores
datm_cdeps_bulk_gefs: 2.5 minutes for the 1-day run using 40 cores
datm_cdeps_control_cfsr: 2.5 minutes for the 1-day run using 40 cores
datm_cdeps_control_gefs: 2.5 minutes for the 1-day run using 40 cores
datm_cdeps_debug_cfsr: 8 minutes for the 6-hr run using 40 cores
datm_cdeps_mx025_cfsr: 6 minutes for the 1-day run using 208 cores
datm_cdeps_mx025_gefs: 6 minutes for the 1-day run using 208 cores
Co-authors:
turuncu@ucar.edu
Denise.Worthen.noaa.gov