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/ufs nst #589

Merged
merged 6 commits into from
Apr 8, 2021
Merged

Feature/ufs nst #589

merged 6 commits into from
Apr 8, 2021

Conversation

XuLi-NOAA
Copy link
Contributor

@XuLi-NOAA XuLi-NOAA commented Mar 8, 2021

Three changes and one bug fix in the NSST model in the coupled frame are ready to merge, see #537.

The new baseline results are at: /scratch1/NCEPDEV/stmp4/Xu.Li/FV3_RT/REGRESSION_TEST_INTEL

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 pretty straightforward, only one change needed as far as I can see.

logical, dimension(im), intent(in) :: wet, icy
real (kind=kind_phys), intent(in) :: rlapse, tgice
real (kind=kind_phys), dimension(im), intent(in) :: oro, oro_uf
integer, intent(in) :: nstf_name1, nstf_name4, nstf_name5
real (kind=kind_phys), dimension(im), intent(in) :: xt, xz, &
real (kind=kind_phys), dimension(im), intent(inout) :: xt, xz, &
Copy link
Collaborator

Choose a reason for hiding this comment

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

If the intent is changed here, also need to change in the metadata (unless the metadata already has intent = inout for these 6 variables.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If the intent is changed here, also need to change in the metadata (unless the metadata already has intent = inout for these 6 variables.

Dom,

Thanks!
Actually, no need to change intent here (I did that for debugging, I think). Will use (in), instead of (inout), for these 6 variables in sfc_nst_post_run.

@binli2337
Copy link
Contributor

@XuLi-NOAA This PR is behind the master branch and needs to be updated.

@@ -671,6 +680,24 @@
type = logical
intent = in
optional = F
[tgice]
Copy link
Collaborator

Choose a reason for hiding this comment

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

It seems tgice is set as a constant (271.2). It is not the actual freezing point temperature of seawater but the minimum allowed temperature of sea water (~ -0.054*salinity) assuming a maximum salinity in the ocean of about 36 ppt.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@XuLi-NOAA didn't make up these names, the were defined previously in GFS_typedefs.meta and CCPP other physics schemes.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@climbfuji I see. Thanks. I do think the naming is not quite correct in any case.

Copy link
Collaborator

Choose a reason for hiding this comment

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

If so, then we should record an issue in ccpp-physics or fv3atm to change this in a future commit. Thanks for spotting this!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If so, then we should record an issue in ccpp-physics or fv3atm to change this in a future commit. Thanks for spotting this!

@climbfuji @DeniseWorthen : I just use tgice which is available, as DOM pointed out. Yes, it is a constant as a safeguard. A salinity dependent frozen temperatures are better or is there an even better way? The naming can be updated when ready.

@@ -773,7 +775,7 @@ end subroutine sfc_nst_post_finalize
! \section NSST_detailed_post_algorithm Detailed Algorithm
! @{
subroutine sfc_nst_post_run &
& ( im, rlapse, tgice, wet, icy, oro, oro_uf, nstf_name1, &
& ( im, kdt, rlapse, tgice, wet, icy, oro, oro_uf, nstf_name1, &
Copy link
Collaborator

Choose a reason for hiding this comment

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

Adding kdt is not necessary here, but it doesn't matter right now, given that all the regression tests have been run at this time.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Adding kdt is not necessary here, but it doesn't matter right now, given that all the regression tests have been run at this time.

@climbfuji : True. It was done for debugging originally, and then I think it is OK to keep it.

Copy link
Collaborator

Choose a reason for hiding this comment

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

We will remove it again in the future, but it is not worth rerunning all the tests because of such a simple change now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Got it.

@binli2337
Copy link
Contributor

@climbfuji : ready to merge

@climbfuji climbfuji merged commit 990b9d0 into NCAR:master Apr 8, 2021
@climbfuji
Copy link
Collaborator

@binli2337 @XuLi-NOAA merged. Please update your submodule pointer for ccpp-physics and revert your `.gitmodules change in your fv3atm PR. The new hash that the ccpp-physics submodule in fv3atm must point to is 990b9d0 (in the NCAR repository, head of master branch). Thanks!

@XuLi-NOAA XuLi-NOAA deleted the feature/ufs-nst branch April 16, 2021 03:22
HelinWei-NOAA pushed a commit to HelinWei-NOAA/ccpp-physics that referenced this pull request Feb 26, 2023
* Add convective cloud to radiation, Thompson MP only.
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.

4 participants