-
Notifications
You must be signed in to change notification settings - Fork 416
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 Galvez-Davison Index calculation #3121
Conversation
Thanks for the contribution @alexander-lakocy! I was doing some quick validation and found the GDI calculated via NCEP and for 00 UTC on 14 September 2023 get: Then using the calculation provided in this PR I get the following: Generally the same look and shape to areas, but the magnitude appears to be a bit different. Do you know of someplace that has this calculated on a grid that would be available for a more detailed comparison (e.g., beyond the visual look at two different plots)? |
I agree the magnitude seems off. It would be useful to have a set of calculated values for creating a useful test. There were not any data grids in the source paper (although I believe they had some sample maps). Maybe we can check with the issue author to see if they have any example test problems? |
It turns out that with the GFS it was using the p_sfc = 1 Pa! The calculation calls for the surface pressure, so when calculating the value for a 1D vertical profile with monotonically increasing values, the current PR captures that with the line
Which would be all good. However, when calculating with the values from the GFS the pressure values are organized in the opposite way with the lowest pressure first and the highest pressure last. Additionally, the maximum value from that will not give the true surface pressure as that would be a separate variable. So we have a choice as to whether to mandate the input of the surface pressure explicitly (the most robust option), or have that be optional and assume a value by using the vertical pressure given and finding the maximum value (for a grid that would likely be 1000 hPa). I think I lean toward adding a fourth parameter to be true to the calculation and that would just add an extra input for someone doing the calculation on a 1D profile. Additionally, the paper states that the gamma parameter should have units of 1/K, but it really should be 1/K^2, which them makes all the units work out and there would be no need to get the magnitude of all of the components and attach dimensional units at the end. I'll put a few comments in the code with some of the changes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lots of small requested changes that identify the issues brought up in a previous comment.
@alexander-lakocy just a quick ping to see if you would like to update this PR with the requested changes or would you rather we take care of the requested changes? |
This looks awesome, I'm glad you were able to track down the issues! I
won't have time this week to make the changes, please feel free to make any
of the requested modifications.
…On Wed, Sep 20, 2023 at 4:31 PM Kevin Goebbert ***@***.***> wrote:
@alexander-lakocy <https://github.com/alexander-lakocy> just a quick ping
to see if you would like to update this PR with the requested changes or
would you rather we take care of the requested changes?
—
Reply to this email directly, view it on GitHub
<#3121 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/ACLVETGPXGNIBSP6NC3KEKLX3NOB5ANCNFSM6AAAAAA2MFAV4Y>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With @kgoebber's additions, I'll step in to review the original code and the updates. A question to kick it off, I'll be back for more!
@dcamron reviewing the references within the GDI paper to dig into the value for C_p and L_v, it would appear most appropriate to use the constant values that are a part of MetPy. I don't imagine that there will be a very large difference in the calculated GDI, but the reference to Betts and Dugan (1974) and from there to Hilton (1972) would indicate the typical values that are traditionally used would be ~1004 J/kg/K and 2.5e6 J/kg for C_p and L_v, respectively. |
Perfect, thanks for digging that up! |
There's now a conflict here with |
okay, I rebased and resolved the conflict. |
Tests are failing now, not sure why? |
I think what we might be seeing there is the difference between the C_p values used. Currently this PR is updated to use the MetPy value and not the value from the paper. What may have happened is that I switched to using the MetPy value after I last pushed to the branch, but didn't actually follow through and update the tests. |
I've updated the test values. I think the old values for the xarray test were the ones that used the wrong surface/starting pressure. I did a quick test with the different values of Cp_d and Lv. The difference with Cp_d are small, a few tenths of the GDI value. The difference with the choice of Lv is much more substantial. I believe the paper uses a value greater than 2.5e6 J/kg because of the index being used for a tropical atmosphere and with the temperature dependence on the latent heat of vaporization they chose not to use the value valid for 0C. The current implementation uses the value from the paper for Lv (which is the variable |
I am alright with that given the tropical context, and thanks for your work to investigate that. I'm going to take a small crack at some implementation changes today, but nothing to hold it up on if I don't quite get there. I'll ping Ryan for a final review then. |
Apply subjective code style cleanup to reduce repetition. Test 950 level check sooner. Collapse array dimension handling.
0616bbb
to
893a602
Compare
Updated and contributed to since this review.
Description Of Changes
Adds calculation for Galvez-Davison Index for tropical convection.
Checklist
/docs/_templates/overrides/metpy.calc.rst
/docs/examples/calculations/Sounding_Calculations.py
This is all visible in the changelist but wanted to document the things I had to change. I didn't see any of this in the Contributor's Guide, ideally we can add it under documentation to make it easier for contributors to document future feature additions.