-
Notifications
You must be signed in to change notification settings - Fork 46
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
Update NEMS for CDEPS component #92
Conversation
src/module_EARTH_GRID_COMP.F90
Outdated
@@ -3596,8 +3599,9 @@ subroutine SetModelServices(driver, rc) | |||
file=__FILE__, rcToReturn=rc) | |||
return ! bail out | |||
#endif | |||
elseif (trim(model) == "datm") then | |||
#ifdef FRONT_DATM | |||
elseif (trim(model) == "datm" .or. trim(model) == "datm_cfsr" .or. & |
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.
Are these CDEPS data atmosphere? I think this implementation is not flexible enough because you need to add a line for each specific datm stream. At this level model only need to know the datm not the stream used in CDEPS. So, only datm is enough for driver at this point. Please refer to following version of the file found in hafs-community#3
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.
Yes, we need to remove datm_cfsr
and datm_gefs
because the way to configure the DATM input data is via its namelist. Moreover, the driver does not need to know what the source of the data is. Also, this approach means we'd need to change the driver to add a new data source to CDEPS. All good reasons why we only need DATM
at the driver level. This is important to get right.
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.
@rsdunlapiv So you're saying that nems.configure will have the atm model either datm (=nemsdatm), datm_cfsr (=cdeps,cfsr source) or datm_gefs (cdeps,gefs source). There is nothing in the driver it self that is needed to distinguish those options.
src/module_EARTH_GRID_COMP.F90
Outdated
elseif (trim(model) == "datm") then | ||
#ifdef FRONT_DATM | ||
elseif (trim(model) == "datm" .or. trim(model) == "datm_cfsr" .or. & | ||
trim(model) == "datm_gefs") then |
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.
This is same with the previous one.
Maybe "datm" is also used as "ATM_model" in nems.configure for cdeps datm,
but inside CDEPS it has its own namalist to specify which source to use.
…On Thu, Mar 18, 2021 at 12:47 PM Denise Worthen ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In src/module_EARTH_GRID_COMP.F90
<#92 (comment)>:
> @@ -3596,8 +3599,9 @@ subroutine SetModelServices(driver, rc)
file=__FILE__, rcToReturn=rc)
return ! bail out
#endif
- elseif (trim(model) == "datm") then
-#ifdef FRONT_DATM
+ elseif (trim(model) == "datm" .or. trim(model) == "datm_cfsr" .or. &
@rsdunlapiv <https://github.com/rsdunlapiv> So you're saying that
nems.configure will have the atm model either datm (=nemsdatm), datm_cfsr
(=cdeps,cfsr source) or datm_gefs (cdeps,gefs source). There is nothing in
the driver it self that is needed to distinguish those options.
—
You are receiving this because your review was requested.
Reply to this email directly, view it on GitHub
<#92 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AI7D6TKNCPFXORDYL2UPKPLTEIVCDANCNFSM4ZA4BGBA>
.
|
I think there is no need to provide datm or datm_cfsr to nems.configure. Driver does not need to know which data stream is used. This is already controlled by data component specific namelist files. So, I am using datm for data atmosphere for all the configurations including NEMS DATM and works smoothly.
… On Mar 18, 2021, at 10:47 AM, Denise Worthen ***@***.***> wrote:
@DeniseWorthen commented on this pull request.
In src/module_EARTH_GRID_COMP.F90 <#92 (comment)>:
> @@ -3596,8 +3599,9 @@ subroutine SetModelServices(driver, rc)
file=__FILE__, rcToReturn=rc)
return ! bail out
#endif
- elseif (trim(model) == "datm") then
-#ifdef FRONT_DATM
+ elseif (trim(model) == "datm" .or. trim(model) == "datm_cfsr" .or. &
@rsdunlapiv <https://github.com/rsdunlapiv> So you're saying that nems.configure will have the atm model either datm (=nemsdatm), datm_cfsr (=cdeps,cfsr source) or datm_gefs (cdeps,gefs source). There is nothing in the driver it self that is needed to distinguish those options.
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub <#92 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAJMBR4TN2ROPEJTH6GYDO3TEIVCDANCNFSM4ZA4BGBA>.
|
@junwang-noaa @DeniseWorthen yes, I think @junwang-noaa has this right. The driver just knows that there is a data atmopshere component that must be included and the run sequence says where to call it. At a lower level in the configuration is where DATM is actually configured with some specific input data streams. |
OK, thanks. I think we're all on the same page here? Do we actually need any changes to module_EARTH_GRID_COMP.F90? |
The NEMS_datm and CDEPS_DATM (here datm_cfsr or datm_gefs) use different land-sea masks. Once NEMS_datm is removed, this temporary fix can also be removed. Please also see the temporary fix in med_map_mod.F90 in CMEPS. |
@binli2337 I think I understand your point. But if you used
Then no change would be required to this routine when we drop the nemsDATM. Is that right? |
Bin, I think the current CMEPS version works for other CDEPS sources, does
this mean "datm_cfsr" and "datm_gefs" are different from other CDEPS data
sources? And other CDEPS data sources are working the same way as NEMSdatm?
…On Thu, Mar 18, 2021 at 2:19 PM Denise Worthen ***@***.***> wrote:
@binli2337 <https://github.com/binli2337> I think I understand your
point. But if you used
if (model(1:4) == "datm") then
Then no change would be required to this routine when we drop the
nemsDATM. Is that right?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#92 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AI7D6TN4OH7LUNXQ4RPHA4DTEI74TANCNFSM4ZA4BGBA>
.
|
@junwang-noaa , The current CMEPS version works for other CDEPS sources only under "hafs" coupling mode. |
* The name of the NEMS data atmosphere model is changed to nems_datm. * The name of the CDEPS atmosphere model will be datm.
@binli2337 I think that the masking of the data component can be modified by changing mask variable in ESMF Mesh file but the only important point is that the masking of the components are handled in the CMEPS in the following line https://github.com/ESCOMP/CMEPS/blob/ad106b0208739b819dba98a88225edfd6e093773/mediator/med_map_mod.F90#L279 and currently we set it as 1 for datm (ERA5). So, you might want to be consistent with that. This part of code needs to be more generic and requires work in CMEPS side. |
@DeniseWorthen @junwang-noaa @rsdunlapiv @uturuncoglu In the past, when CICE5 and MOM5 were removed, the parts related to cice5 and mom5 remained in the module_EARTH_GRID.COM.F90 file. When nems_datm is removed in the future, the nems_datm related parts can also remain in the module_EARTH_GRID_COM.F90 file. |
Bin, I saw there is lots of legacy code in NEMS, I think we do need to
clean them up since the components are not available in UFS.
…On Mon, Mar 22, 2021 at 9:14 AM BinLi-NOAA ***@***.***> wrote:
@DeniseWorthen <https://github.com/DeniseWorthen> @junwang-noaa
<https://github.com/junwang-noaa> @rsdunlapiv
<https://github.com/rsdunlapiv> @uturuncoglu
<https://github.com/uturuncoglu>
I have revised module_EARTH_GRID_COMP.F90 to separate "nems_datm" and
"datm" (used for CDEPS). Please take a look to see if the changes are OK.
In the past, when CICE5 and MOM5 were removed, the parts related to cice5
and mom5 remained in the module_EARTH_GRID.COM.F90 file.
When nems_datm is removed in the future, the nems_datm related parts can
also remain in the module_EARTH_GRID_COM.F90 file.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#92 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AI7D6TNDFKQZ7S5DAI75BI3TE47CFANCNFSM4ZA4BGBA>
.
|
src/module_EARTH_GRID_COMP.F90
Outdated
elseif (trim(model) == "datm") then | ||
#ifdef FRONT_DATM | ||
elseif (trim(model) == "nems_datm") then | ||
#ifdef FRONT_NEMS_DATM |
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.
@binli2337 @junwang-noaa I think we don't need to have same code twice in here. You could use #if defined FRONT_NEMS_DATM || defined FRONT_CDEPS_DATM
and remove extra if statement in here. This is already working under HAFS fork.
fixes: #99