Skip to content
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

Add new PBL diagnostic variables dkt and dku #607

Merged
merged 26 commits into from
Apr 21, 2021

Conversation

BinLiu-NOAA
Copy link
Collaborator

@BinLiu-NOAA BinLiu-NOAA commented Apr 2, 2021

Add new PBL diagnostic variables dkt and dku (atmosphere heat and momentum diffusivity)

Contributors: @ChunxiZhang-NOAA, @AndrewHazelton, @BinLiu-NOAA

This PR addresses ccpp-physics issue #606 .
Related PRs:
ufs-community/ufs-weather-model/pull/505
NOAA-EMC/fv3atm/pull/269

BinLiu-NOAA and others added 20 commits June 24, 2020 15:05
Cleanup trailing white space for some hwrf physics related source code
…s_20200619

Update support/HAFS from NCAR dtc/hwrf-physics 2020/06/19
@BinLiu-NOAA BinLiu-NOAA marked this pull request as ready for review April 2, 2021 15:56
@BinLiu-NOAA BinLiu-NOAA changed the title Add a new PBL diagnostic variable dkudiagnostic Add new PBL diagnostic variables dkt and dku Apr 16, 2021
Copy link
Collaborator

@climbfuji climbfuji left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great, just one change that needs to be made, a leftover from the previous code

*Add dkt as a diagnostic variable for satmedmfvdifq.F.
Contributors: Andrew Hazelton, Chunxi Zhang, Bin Liu
@BinLiu-NOAA BinLiu-NOAA force-pushed the feature/dkudiagnostic branch from bd89cbc to 8c44643 Compare April 16, 2021 16:27
Copy link
Collaborator

@climbfuji climbfuji left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good now, thanks! Will approve when it is time for this PR to go in.

…iables, so

that they will not be conflicted with other similar variables in GFS_typedefs.meta.
Contributors: Chunxi Zhang and Bin Liu
@@ -420,14 +420,23 @@
intent = inout
optional = F
[dkt]
standard_name = atmosphere_heat_diffusivity
standard_name = atmosphere_heat_diffusivity_from_gfspbl
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please remove _from_gfspbl here and also for [dku]. Same in physics/satmedmfvdifq.meta and in GFS_typedefs.meta. There is no reason why these variables should be specific to GFS PBL.

Copy link
Collaborator Author

@BinLiu-NOAA BinLiu-NOAA Apr 19, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@climbfuji We actually were using the names without adding "_from_gfspbl" in the dkt/dku stand_names originally, However, the model build failed due to the complaining that there are duplicated standard names of "atmosphere_heat_diffusivity" in ccpp/data/GFS_typedefs.meta.
As you can see, there is already a dkt under the GFS_interstitial_type section with a standard name of "atmosphere_heat_diffusivity". After looking into it a little more, we saw some examples on how to use different names for the same/similar variables in this GFS_typedefs.meta file, for example:
standard_name = instantaneous_atmosphere_heat_diffusivity
standard_name = atmosphere_heat_diffusivity_background
standard_name = atmosphere_heat_diffusivity_background_maximum
standard_name = atmosphere_heat_diffusivity_from_shoc
standard_name = atmosphere_heat_diffusivity_for_mynnpbl
standard_name = atmosphere_heat_diffusivity
With that, we introduced the
standard_name = atmosphere_heat_diffusivity_from_gfspbl
for dkt (did the same for dku). After that the build worked fine. Agree that, this is not an elegant way and sometime confusing to use different standard_names for the same variable. It would be nice to standardize or unify this from the model and ccpp interface side. Please let us know if you have any comments/suggestions. Thanks!

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@climbfuji We actually were using the names without adding "_from_gfspbl" in the dkt/dku stand_names originally, However, the model build failed due to the complaining that there are duplicated standard names of "atmosphere_heat_diffusivity" in ccpp/data/GFS_typedefs.meta.
As you can see, there is already a dkt under the GFS_interstitial_type section with a standard name of "atmosphere_heat_diffusivity". After looking into it a little more, we saw some examples on how to use different names for the same/similar variables in this GFS_typedefs.meta file, for example:
standard_name = instantaneous_atmosphere_heat_diffusivity
standard_name = atmosphere_heat_diffusivity_background
standard_name = atmosphere_heat_diffusivity_background_maximum
standard_name = atmosphere_heat_diffusivity_from_shoc
standard_name = atmosphere_heat_diffusivity_for_mynnpbl
standard_name = atmosphere_heat_diffusivity
With that, we introduced the
standard_name = atmosphere_heat_diffusivity_from_gfspbl
for dkt (did the same for dku). After that the build worked fine. Agree that, this is not an elegant way and sometime confusing to use different standard_names for the same variable. It would be nice to standardize or unify this from the model and ccpp interface side. Please let us know if you have any comments/suggestions. Thanks!

Oh, I see. Not your fault at all, just terrible precedence. In this case, I suggest to leave the PR as-is, let's clean this up afterwards.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, some confusion about what was going on with these similarly-named variables is why I made a new one "dkudiagnostic" for momentum diffusivity, but that's obviously not the best solution memory-wise. Definitely would be good if we can help clean this up in the future hopefully.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@climbfuji, With some further discussions and code digging with @ChunxiZhang-NOAA, it looks like our current solution for diagnostic dkt variable will probably break the the existing interstitial dkt usage in physics/moninedmf.f[meta] and in ccpp/data/GFS_typedefs.F90[meta]. Currently, we were thinking of the following two potential solutions:

  • A. Using dkt_diag and dku_diag (atmosphere_heat_diffusivity_diag and atmosphere_momentum_diffusivity_diag) for the the diagnostic output so that they are not conflict with other usages (interstitial, coupling, etc.);
  • B. Currently only outputting dku (atmosphere_momentum_diffusivity) as a new diagnostic variable (which should not be conflicted with other dku usage at this moment), while dropping off the proposed dkt diagnostic output.

Appreciate it if you could provide any insight/recommendation on how to move forward with this change. Thanks!

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@climbfuji Yes, this is also a good solution. I searched out the usage of "Interstitial%dkt" inside ufs-weather-model. These are the locations the Interstitial%dkt was used:

grep -ir "Interstitial%dkt" *
FV3/ccpp/data/GFS_typedefs.F90: allocate (Interstitial%dkt (IM,Model%levs-1))
FV3/ccpp/data/GFS_typedefs.F90: Interstitial%dkt = clear_val
FV3/ccpp/data/GFS_typedefs.F90: write (0,*) 'sum(Interstitial%dkt ) = ', sum(Interstitial%dkt )
FV3/ccpp/physics/physics/GFS_debug.F90: call print_var(mpirank, omprank, blkno, Grid%xlat_d, Grid%xlon_d, 'Interstitial%dkt ', Interstitial%dkt )

So, if we move the dkt into the Diag%dkt section. Then the above will be the places need to be replaced by Diag%dkt, right? Of course, in this case the dkt and dku can then use the names of atmosphere_heat_diffusivity and atmosphere_momentum_diffusivity. Do you think this will work? Thanks!

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think so, yes. The whole point moving it from Interstitial% to Diag% is to make it persistent so that it can be written out. In GFS_debug.F90, it will need to go from module GFS_interstitialtoscreen to GFS_diagtoscreen, but that should be pretty much self-explanatory in the file.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @climbfuji! I have just made and pushed the changes into this branch and in the FV3atm side as well. Please help to check/review the changes when you get a chance. Thanks!

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, let's get started with testing this today, your PR will go next after the current LAM post commit.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok. Just synced this branch with the latest master to get it ready for model level RT.

…ard_names

for dkt/dku back to atmosphere_heat/momentum_diffusivity.
@BinLiu-NOAA BinLiu-NOAA force-pushed the feature/dkudiagnostic branch from 62b53a2 to 28c815a Compare April 20, 2021 02:17
@BrianCurtis-NOAA
Copy link
Contributor

BrianCurtis-NOAA commented Apr 21, 2021

Testing with ufs-weather-model completed and passed.

@climbfuji
Copy link
Collaborator

In a follow-up PR, we will change the dimensions (including metadata) of these arrays to the full vertical_dimension = km, and make the corresponding changes in fv3atm. This is to avoid out-of-bounds errors when writing the new diagnostic errors to file.

@climbfuji climbfuji merged commit 3dbae5d into NCAR:master Apr 21, 2021
@BinLiu-NOAA BinLiu-NOAA deleted the feature/dkudiagnostic branch June 29, 2021 16:57
HelinWei-NOAA pushed a commit to HelinWei-NOAA/ccpp-physics that referenced this pull request Feb 26, 2023
* Change ccpp-physics branch to fix_tq2m_diagnoahmp
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants