Updates for component land model #94
Conversation
uturuncoglu
requested review from
barlage,
cenlinhe,
grantfirl,
Qingfu-Liu,
dustinswales and
mzhangw
as code owners
| @@ -196,6 +196,8 @@ physics/ysuvdif.* @Qingfu-Liu @WeiguoWang-NOAA | |||
| physics/zhaocarr_gscond.* @RuiyuSun @grantfirl @Qingfu-Liu @dustinswales | |||
| physics/zhaocarr_precpd.* @RuiyuSun @grantfirl @Qingfu-Liu @dustinswales | |||
|
|
|||
| physics/sfc_land.* @uturuncoglu @barlage | |||
There was a problem hiding this comment.
@uturuncoglu Since you're adding yourself to the CODEOWNERS, please accept the collaboration invitation for this repository that I just sent you so that you will be added as a reviewer automatically when these files are touched in a PR.
There was a problem hiding this comment.
@grantfirl Sorry. I did not do it intentionally. I was just trying to follow the conventions in there. Since I am not doing to much development in CCPP side, you could remove me from the list. That is totally fine.
There was a problem hiding this comment.
@uturuncoglu IMO, it's totally appropriate to add you since you're adding these files to the repository. If @barlage is OK with being the CODEOWNER by himself, that is OK too. I'll let you two decide what you'd prefer.
physics/noahmpdrv.meta
Outdated
| @@ -633,6 +633,20 @@ | |||
| dimensions = () | |||
| type = logical | |||
| intent = in | |||
| [cpllnd] | |||
| standard_name = flag_for_land_coupling | |||
There was a problem hiding this comment.
We've been moving to do_* rather than flag_for*, but since flag_for_land_coupling already exists and there are several others like it, it seems unfair to ask you to change it everywhere. This will have to be part of a separate standard name cleanup at some later point, I guess.
There was a problem hiding this comment.
@grantfirl That is fine. I could fix them at least for land component coupling related once. When I look at to the examples for it, I found do_mynnsfclay but it's standard name is start with flag_for. Do you have any convention for it or just changing the argument name is fine at this point. Anyway, let me know what you think?
There was a problem hiding this comment.
Ya, I was talking about standard names only. I think that the variable names in the code are fine and the CCPP doesn't care. But, like I said, I wouldn't worry about changing it now since there is a lot of cleanup in other places to do anyway. The comment was more for the future, if you're going to be adding more on/off logicals, I think that having the standard names start with do_X is what the CCPP physics management committee prefers.
There was a problem hiding this comment.
@grantfirl Okay. I am not doing those changes at this point. Thanks.
There was a problem hiding this comment.
@uturuncoglu I suggest changing the name of the new module, from sfc_land to something like noahmp_post?
Also, since this is a new module, is there a need to use fortran 77?
|
@dustinswales there is no need to use F77. I just follow the way with others similar such as sfc_ocean. In terms of changing name, I think it is better with this way since as I know there is other project to bring other external land component (GFDL's LM4) to the system. So, I prefer see the name generic to support also other developments in terms of land models. But, if you replay want to change it, I could do it easily. Let me know what you think? |
|
@uturuncoglu We can leave the naming for another day. However, since this is new code, and it's a "ccpp-entrypoint", and there aren't many lines in this module, can you update to F90? (see https://ccpp-techdoc.readthedocs.io/en/v6.0.0/CompliantPhysicsParams.html#coding-rules) |
|
@dustinswales Okay. I changed it to F90 and clean the module. Please check it again and let me know if you see anything else. Thanks. |
|
Due to schedule issue of priority PRs, we are trying to combine #161 to this. A justification is that baseline cases changes are orthogonal: land vs rrfs case changes. @grantfirl Would it be ok? @uturuncoglu I will keep posting the decision. |
It's not ideal, but it OK with me. |
|
@uturuncoglu can you combine in #161 to this pr? @grantfirl thanks for the quick reply! |
|
@jkbk2004 yes, that is fine for me. |
@uturuncoglu There are changes to combine at FV3 level as well: ccpp/data and ccpp/driver levels at NOAA-EMC/fv3atm#773. If you feel it's not ideal, we can commit separately. |
|
@jkbk2004 That is fine for me. As I see there is no ant conflict between these two PR. Right? |
@uturuncoglu I don't see major conflicts at the level of ccpp/data and ccpp/driver. |
|
@jkbk2004 yes, me too. I think they could be merged together easily. |
|
@uturuncoglu Let me know if you need any help merging #161 into your branch. |
|
@grantfirl Okay. Let me merge it and test in my side quickly. BTW, Orion and Hercules are down. So, I'll use Derecho for testing. |
|
@grantfirl @jkbk2004 Okay. I merged ccpp/physics one. Do I need to merge NOAA-EMC/fv3atm#773 for the FV3 also. |
@uturuncoglu Cool! Please, go ahead to combine NOAA-EMC/fv3atm#773 to your NOAA-EMC/fv3atm#682. |
|
@jkbk2004 Did it. I am running land related RT on Derecho to be sure that are still working. It looks fine at this point. |
|
@grantfirl testing is complete one ufs-wm PR #1845. Can you merge this PR for us, please? |
This PR aims to bring fully coupled atmosphere-land component configuration to UFS Weather Model. The newly added
sfc_land.[F90|meta]is mainly responsible to receive information from component land model (NoahMP).