-
Notifications
You must be signed in to change notification settings - Fork 425
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
Update e_s to a more accurate formulation and add e_i (based on Murphy and Koop, 2005) #2480
base: main
Are you sure you want to change the base?
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.
The code looks fine to me, though I haven't looked at the physical side yet.
#3726 is another PR along this line, if you want to poke at that
|
||
@exporter.export | ||
@preprocess_and_wrap(wrap_like='temperature') | ||
@process_units({'temperature': '[temperature]'}, '[pressure]') |
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.
Perhaps
@process_units({'temperature': '[temperature]'}, '[pressure]') | |
@process_units({'temperature': 'kelvin'}, '[pressure]') |
to ensure it converts if someone passes Celsius or Fahrenheit? I should check whether the MetPy decorator works like the pint one.
def test_ice_sat_vapor_pressure(): | ||
"""Test ice_saturation_vapor_pressure calculation.""" | ||
temp = (np.arange(1, 38) * units.degC).to(units.degK) |
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 it be worth parametrizing the test to pass temperature once in Kelvin and once in Celsius to ensure the conversion works properly? Something like:
def test_ice_sat_vapor_pressure(): | |
"""Test ice_saturation_vapor_pressure calculation.""" | |
temp = (np.arange(1, 38) * units.degC).to(units.degK) | |
@pytest.mark.parametrize('unit', [units.degK, units.degC]) | |
def test_ice_sat_vapor_pressure(unit): | |
"""Test ice_saturation_vapor_pressure calculation.""" | |
temp = (np.arange(1, 38) * units.degC).to(unit) |
Description Of Changes
In this pull request, I've modified the equilibrium vapor pressure (e_s) calculation to a more accurate formulation based on Murphy and Koop (2005; reference added). I've also added a method to calculate the ice equilibrium vapor pressure (e_i) based on the same manuscript as well as a unit test for this method. Different variations (using different input coefficients) of Teten's formula have been used in the literature (e.g., Bolton, 1980; Alduchov and Eskrige, 1996) but the use of the exponential form of this type of parameterization means that the fits are valid in a relatively limited range of temperatures. The MP2005 formulation on the other hand is valid for a much broader range of temperatures.
I was contemplating whether to add an optional input parameter to saturation_vapor_pressure for the water phase with a default to ice, but then thought that it might be better to add a separate method for ice, thus making code and documentation easier to follow.
Also, I would recommend using the "equilibrium vapor pressure" term rather than "saturation vapor pressure" as that is a more physically accurate term, but that would likely result in plenty of mess after changing the terminology and method terms.
Checklist