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

Draft for adding support for saturation water vapour over ice #2323

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

joernu76
Copy link
Contributor

@joernu76 joernu76 commented Feb 1, 2022

Particularly improving usefullness of RH at low temperatures

See #2320

Description Of Changes

Checklist

  • Closes #xxxx
  • Tests added
  • Fully documented

Particularly improving usefullness of RH at low temperatures

See Unidata#2320
@joernu76 joernu76 requested a review from a team as a code owner February 1, 2022 11:08
@joernu76 joernu76 requested review from dopplershift and removed request for a team February 1, 2022 11:08
@joernu76 joernu76 marked this pull request as draft February 1, 2022 11:09
@@ -25,7 +25,7 @@
@exporter.export
@preprocess_and_wrap(wrap_like='temperature', broadcast=('temperature', 'dewpoint'))
@check_units('[temperature]', '[temperature]')
def relative_humidity_from_dewpoint(temperature, dewpoint):
def relative_humidity_from_dewpoint(temperature, dewpoint, phase='liquid'):
Copy link
Contributor

@DWesl DWesl Feb 20, 2025

Choose a reason for hiding this comment

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

You'll need to add the new parameter to the documentation before un-drafting

I don't know maintainer policy on magic string constants vs. instances of enum.Enum, though the latter would aid discoverability

)

def ice(temperature):
# Alduchov and Eskridge (1996)
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd appreciate a full citation in the References section of the docstring

Comment on lines +1027 to +1031
def liquid(temperature):
# Converted from original in terms of C to use kelvin.
return mpconsts.nounit.sat_pressure_0c * np.exp(
17.67 * (temperature - 273.15) / (temperature - 29.65)
)
Copy link
Contributor

@DWesl DWesl Feb 21, 2025

Choose a reason for hiding this comment

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

Given this is what was there before, why not rename saturation_vapor_pressure to _saturation_vapor_pressure_over_liquid, create a new _saturation_vapor_pressure_over_ice, and create a new saturation_vapor_pressure that just dispatches to one of those?

It would allow skipping the dispatch if something needed one or the other, though I don't know if that's enough of a reason for the maintainers

Copy link
Contributor

Choose a reason for hiding this comment

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

Found request by maintainer to avoid new functions.

Perhaps replace the function definition line with if phase != "ice", assign the result to saturation_over_liquid, and return that immediately if phase == "liquid"? (and similarly for ice just below)

Comment on lines +1046 to +1048
t_sel = (temperature > 250.16) & (temperature < 273.16)
alpha[t_sel] = ((temperature[t_sel] - 250.16) / (273.16 - 250.16)) ** 2
alpha[temperature >= 273.16] = 1
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it be worth adding these temperatures to metpy.constants and using those here?

Copy link
Contributor

@DWesl DWesl left a comment

Choose a reason for hiding this comment

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

Does the paper you cite have specific values you can test?

I do remember saturation vapor pressure over liquid and over ice crossed over at freezing, I think saturation vapor pressure over liquid was higher than saturation vapor pressure over ice below freezing, which allows a property-based test, at least.

Am I correct in guessing the difference between the two saturation vapor pressures is related to the -15 degrees Celsius dendritic growth maximum? That could be an interesting addition to the gallery demonstrating this functionality

@dopplershift
Copy link
Member

@DWesl note that part of this will likely be superseded by #3726.

@DWesl
Copy link
Contributor

DWesl commented Feb 21, 2025

@DWesl note that part of this will likely be superseded by #3726.

So it would. I had thought from the title it would just be replacing $L_v$ with $L_v(T)$, and so missed the addition of saturation_vapor_pressure_ice

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.

3 participants