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

Added code to perform hail size forecasting diagnostic #164

Merged
merged 8 commits into from
Apr 29, 2022

Conversation

ywangwof
Copy link

Description

FV3-HAILCAST hail size forecasting diagnostic (Adams-Selin and Ziegler 2016 MWR).

Authors: John Henderson and Rebecca Adams-Selin AER, Conrad Ziegler NSSL.

How Has This Been Tested?

It was tested on Jet. In order to turn on the HAILCAST package, this namelist block should be added to input.nml.

&fv_diagnostic_nml
do_hailcast = .true.
/

and this entry should be added to diag_table

"gfs_dyn", "hailcast_dhail_max", "HAILCAST_DHAIL", "fv3_history2d", "all", .false., "none", 2

Checklist:

Please check all whether they apply or not

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • Any dependent changes have been merged and published in downstream modules

@lharris4
Copy link
Contributor

This is a very nice addition to FV3 and I am glad to see that has been contributed back to the community. I have a couple of recommendations:

  1. I would prefer that the diagnostics code and variables all be contained within the one HAILCAST module. In particular the code being put into fv_arrays.F90 and fv_nggps_diag.F90 should all be put within the one module_diag_hailcast.F90, with only an initialization routine and an execution routine being exposed to the rest of the code base. The files fv_arrays.F90 and fv_nggps_diag.F90 are already too large and unwieldy.

  2. Is this implementation of HAILCAST this designed for any particular microphysical scheme? If so this should be explicitly stated and checked for at initialization. Alternately, if the scheme is relatively generic, but needs the microphysics to make available certain fields, this should also be checked for at initialization, with either a warning or a fatal error thrown if the appropriate fields are not found.

Thanks for your contribution to FV3.

@bensonr
Copy link
Contributor

bensonr commented Jan 14, 2022

@ywangwof - @lharris4 requested changes to this PR. If you are unable or unwilling to address these by the end of January 2022, we will be closing this PR.

@ywangwof
Copy link
Author

Hello @bensonr, we are working on the request. Unfortunately, we are occupied by some time-sensitive projects recently. Could we request for an extension to the end of February for this task? Thanks.

@bensonr
Copy link
Contributor

bensonr commented Jan 18, 2022

@ywangwof - we can leave this open.

@lharris4
Copy link
Contributor

lharris4 commented Jan 19, 2022 via email

@lharris4
Copy link
Contributor

This is much better, exactly what I was asking for. As long as this is checked so it compiles and works correctly I would approve this merge. Thanks for working on this.

Copy link
Contributor

@bensonr bensonr left a comment

Choose a reason for hiding this comment

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

I agree with @lharris4, this is a much cleaner implementation. Please see the review comments and address.

model/fv_arrays.F90 Outdated Show resolved Hide resolved
tools/module_diag_hailcast.F90 Outdated Show resolved Hide resolved
@ywangwof
Copy link
Author

Thanks for all your comments. We are still having some issues. After I have fully tested the module, I will ask for your review again.

model/fv_arrays.F90 Outdated Show resolved Hide resolved
@ywangwof
Copy link
Author

Hello @lharris4, @bensonr, please review this PR again and I finally had it tested with Thompson, NSSL 2-moment and GFDL microphysical schemes with the latest develop branch of the "ufs-weather-model". Thanks.

@bensonr bensonr self-requested a review March 22, 2022 14:18
tools/module_diag_hailcast.F90 Outdated Show resolved Hide resolved
enddo
enddo
if(mod(kdtt,nsteps_per_reset)==0)then
do j=jsco,jeco
Copy link
Collaborator

Choose a reason for hiding this comment

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

remove extra spaces?

Copy link
Author

Choose a reason for hiding this comment

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

That is true. I would suggest that this practice should be established as a code convention. Anyway, almost all editors can be set to prune extra spaces at the end of lines automatically before saving.

Furthermore, we swap the orders of the IF statement and the DO loop statements for code efficiency. Thanks.

Copy link
Contributor

@lharris4 lharris4 left a comment

Choose a reason for hiding this comment

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

This is much improved but I still have a question about the hailcast-specific code in the fv_nggps_diag.

driver/fvGFS/fv_nggps_diag.F90 Show resolved Hide resolved
@ywangwof
Copy link
Author

Thanks for your comments and suggestions. Here are the summary about my last commit based on your comments.

  1. I moved the namelist and reading from module_diag_hailcast to fv_nggps_diag_init in fv_nggps_diags_mod;
  2. Listed the variables and subroutines imported from module_diag_hailcast explicitly in fv_nggps_diag.F90;
  3. Removed unnecessary comments as noted by @junwang-noaa;
  4. Replaced all GOTO statements with EXIT statements;
  5. Introduced new variables to replaced some exponent expressions as possible.

Please review them again at your convenience. Thanks again.

@laurenchilutti
Copy link
Contributor

@ywangwof is there a fv3atm and ufs-weather-model PR in place to bring these changes into the UFS weather model?

@ywangwof
Copy link
Author

ywangwof commented Apr 8, 2022

Thanks. @laurenchilutti. I just issued two PRs for them.
fv3atm, NOAA-EMC/fv3atm/pull/518.
ufs-weather-model, ufs-community/ufs-weather-model/pull/1164

@junwang-noaa
Copy link
Collaborator

@laurenchilutti @bensonr FYI. we are running RT in UFS (PR#1156)[https://github.com/ufs-community/ufs-weather-model/pull/1156] with this PR. We will let you know when all the RT test pass, please commit the changes (probably on 4/29). Thanks

@binli2337
Copy link

@laurenchilutti @bensonr All tests passed in UFS (PR#1156). Please commit the changes from PR#164. Thanks.

@laurenchilutti laurenchilutti merged commit fad4c9f into NOAA-GFDL:dev/emc Apr 29, 2022
JiliDong-NOAA added a commit to JiliDong-NOAA/GFDL_atmos_cubed_sphere that referenced this pull request May 3, 2022
Added code to perform hail size forecasting diagnostic (NOAA-GFDL#164)
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.

6 participants