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

Feature/hafs ccpp #390

Closed
wants to merge 3 commits into from
Closed

Conversation

BinLiu-NOAA
Copy link
Collaborator

Connect HAFS version of GFS EDMF PBL scheme with CCPP (from @Qingfu-Liu, Bin, Chunxi, and Weiguo).

Files added/modified:
physics/moninedmf_hafs.f
physics/moninedmf_hafs.meta

Notes:

  • Already synced with the latest master.
  • Tests were conducted in the HAFS system.
  • The FV3 (fv3atm) repository/branch will also need to be updated so that the HAFS_v0_gfdlmp and HAFS_v0_gfdlmp_nocp suites can be used.

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.

I noted two aspects that we would usually ask you to change. However, given that @grantfirl and his colleagues are working on a unification of moninedmf_hafs and moninedmf, I assume they will cover those aspects and I think it is not necessary for you to make those changes. I will try to pull your PR into dtc/develop instead of master to expedite merging it.

!
use machine , only : kind_phys
use funcphys , only : fpvs
use physcons, grav => con_g, rd => con_rd, cp => con_cp &
Copy link
Collaborator

Choose a reason for hiding this comment

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

These constants should be passed in via the argument list.

!> \ingroup PBL
!! \brief Routine to solve the tridiagonal system to calculate temperature and moisture at \f$ t + \Delta t \f$; part of two-part process to calculate time tendencies due to vertical diffusion.
!!
!! Origin of subroutine unknown.
Copy link
Collaborator

Choose a reason for hiding this comment

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

These tridiagonal matrix calculation functions could be merged into tridi.f.

@BinLiu-NOAA
Copy link
Collaborator Author

I noted two aspects that we would usually ask you to change. However, given that @grantfirl and his colleagues are working on a unification of moninedmf_hafs and moninedmf, I assume they will cover those aspects and I think it is not necessary for you to make those changes. I will try to pull your PR into dtc/develop instead of master to expedite merging it.

Hi Dom,

Thanks for the comments and suggestions for moving forward. Looking forward to @grantfirl and Biswas's help to come up an unified moninedmf version.

Bin

@grantfirl
Copy link
Collaborator

@BinLiu-NOAA @climbfuji Both of the concerns raised by Dom are addressed in PR #395 which should be regarded as a followup to this PR. Feel free to look at the code changes there if you want.

I'd like to go ahead and approve this PR to give credit to @BinLiu-NOAA and colleagues for doing this work and to have the HAFS-specific version in the repository if needed later. In addition, having this version in the repo can serve as a basis for regressiont testing PR #395 to make sure the combined version behaves identically to the HAFS-specific version with the hurr_pbl flag turned on and alpha set appropriately, and the GFS version when hurr_pbl is false. I'd like for PR #395 to supercede this one once that testing is satisfactory.

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 PR was pulled into #394 and merged into NCAR's dtc/develop branch. Therefore, this PR will be closed.

@climbfuji climbfuji closed this Feb 6, 2020
@BinLiu-NOAA BinLiu-NOAA deleted the feature/hafs_ccpp branch March 24, 2020 18:41
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.

3 participants