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 a Thickness Calculation #621
Conversation
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.
Thanks for another excellent PR! A couple little comments and the test error are about all that needs addressed.
metpy/calc/thermo.py
Outdated
mixing = kwargs.pop('mixing', np.zeros_like(temperature)) | ||
molecular_weight_ratio = kwargs.pop('molecular_weight_ratio', epsilon) | ||
bottom = kwargs.pop('bottom', None) | ||
depth = kwargs.pop('depth', pressure.max() - pressure.min()) |
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.
I think depth = kwargs.pop('depth', None)
should work here as well.
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.
Unfortunately, get_layer defaults to a depth of 100 hPa when None is specified, which didn't seem to make sense for this function (compared to the data of the whole layer). But, I can definitely still change it to that if you would like.
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.
Good point. Do you really need to call get_layer
if you don’t have a depth
argument then?
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.
That's a good point, I don't think I really need to in that case. So, I've made the get_layer call conditional now in my latest commit.
metpy/calc/thermo.py
Outdated
layer_virttemp = virtual_temperature(layer_temp, layer_w, molecular_weight_ratio) | ||
|
||
# Take the integral and return the result in meters | ||
return (- Rd / g * np.trapz(layer_virttemp / layer_p, x=layer_p) * |
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.
dlnp right?
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.
Thank you for pointing that out...I'm used to the continuous case where d(p)/p and d(ln(p)) are equivalent. But, after playing around with it some more, d(ln(p)) does indeed give the better results compared to what the layer-average form of the hyposometric equation would lead me to expect. I've corrected this in the commit I just made.
metpy/calc/tests/test_thermo.py
Outdated
assert_almost_equal(thickness, 10725.92 * units.m, 2) | ||
|
||
|
||
def test_thickness_hydrostatic_isothermal(): |
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.
Travis error is coming from rounding on legacy here:
E AssertionError:
E Arrays are not almost equal to 2 decimals
E ACTUAL: 5542.1180018995983
E DESIRED: 5542.11
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.
Hopefully this should be fixed with the formula and test updates I was just able to make.
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.
This is an excellent PR! I only have some concerns about whether the handling for optional mixing ratio is a good idea for potentially large datasets.
metpy/calc/thermo.py
Outdated
pressure_to_height_std, virtual_temperature | ||
|
||
""" | ||
mixing = kwargs.pop('mixing', np.zeros_like(temperature)) |
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.
Instead of calculating virtual temp with 0 mixing, could you instead do:
mixing = kwargs.pop(‘mixing’, None)
if mixing is None:
layer_virttemp = temperature
else:
layer_virttemp = virtual_temperature(...)
I appreciate the elegance of your original approach, but I feel like for a large enough grid (and deep enough profile), the array of zeros could have significant overhead for both memory and runtime.
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.
Of course now that makes the get_layer
call conditional as well, but I think that’s worth saving the extra calculation. You’d also need another test case for the case where you’re not passing in w. Thoughts, @jthielen ?
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.
That sounds good to reduce the overhead of the zeros approach. I have made the calls conditional now in my most recent commit, and added tests to cover both situations. Let me know if that is what you were thinking.
metpy/calc/thermo.py
Outdated
mixing = kwargs.pop('mixing', np.zeros_like(temperature)) | ||
molecular_weight_ratio = kwargs.pop('molecular_weight_ratio', epsilon) | ||
bottom = kwargs.pop('bottom', None) | ||
depth = kwargs.pop('depth', pressure.max() - pressure.min()) |
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.
Good point. Do you really need to call get_layer
if you don’t have a depth
argument then?
metpy/calc/thermo.py
Outdated
temperature : `pint.Quantity` | ||
Atmospheric temperature profile | ||
mixing : `pint.Quantity`, optional | ||
Profile of dimensionless mass mixing ratio. Defaults to zero, so that virtual |
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.
Would need to update if the handling of no mixing ratio is changed.
@jrleeman I don’t recall: does |
Hmmm.. I don't remember - I'll have to try it, but I believe it does. (We do use it in some of the indices.) |
In case it's useful, I wasn't able to get import numpy as np
from metpy.calc.tools import get_layer
from metpy.units import units
# Works
pressure = [850, 700, 500] * units.hPa
temperature = [5, 0, -2] * units.degC
print(get_layer(pressure, temperature, bottom=750*units.hPa))
# Fails
pressure = [[850, 700, 500],
[850, 700, 500],
[850, 700, 500]] * units.hPa
temperature = [[5, 0, -2],
[4, 1, 0],
[4, 0, -1]] * units.degC
print(get_layer(pressure, temperature, bottom=750*units.hPa)) Output:
|
This commit adds a thickness calculation using an integral form of the hypsometric equation with virtual temperature adjustment. This assumes hydrostatic balance. Also included are two tests of this calculation, one using matching data from other tests and one using a simple dry isothermal layer.
Based on feedback, corrects the thickness formula to use d(ln(pres)) instead of d(pres)/pres. While these are equivalent in the continuous case, they do not give the same results in the discrete case here. Tests have likewise been updated to account for this change in formula (which should hopefully also resolve the legacy test failure).
Based on review feedback, updates the optional mixing ratio to be handled conditionally rather than with an array of zeros (if none is given), as well as conditionally calling get_layer only when a bottom/depth is specified. Adds two additional tests to cover all four scenarios (full layer-moist, subset-moist, full layer-dry, subset-dry). Also adds a citation for the thickness formula.
The recent addition of the static energy calculations unfortunately caused a messy rebase conflict where the new functions got interleaved. I thought I got it all cleaned up, but unfortunately missed this exporter statement.
Yep - My remembering was incorrect. It isn't grid compatible |
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.
I think this is in good shape now. Thanks for making those changes. Good @jrleeman ?
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.
LGTM
Thanks for the help! 🎉 |
A pull request working on issue #295.
This adds a thickness calculation using an integral form of the hypsometric equation with virtual temperature adjustment. Also included are two tests of this calculation, one using matching data from other tests and one using a simple dry isothermal layer.
I don't yet have any text citations for the formula (I was just pulling from my notes), but once I get back to campus I should be able add that if needed.
Any feedback is greatly appreciated!