-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Calculate Relative Humidity via Magnus Tetens Equation #2286
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
Conversation
@kurt-rhee Perhaps it would be better to continue the discussion in #1744 rather than start a new thread here. |
@adriesse good idea, I've copied the comment above to the thread in question |
…, changed type hints in docstring, changed to suggested coefficient format
Co-authored-by: Anton Driesse <[email protected]>
@adriesse agreed, anecdotal and unnecessary, it has been removed. |
I have left the formatting of functions outside of the tdew and rh conversion functions. I can format them to satisfy flake8 linter if that is desired by maintainers. Just let me know how I can help.
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.
Some more comments. A what's new entry will be needed before too long!
Co-authored-by: Kevin Anderson <[email protected]>
Co-authored-by: Kevin Anderson <[email protected]>
Co-authored-by: Kevin Anderson <[email protected]>
Co-authored-by: Kevin Anderson <[email protected]>
Co-authored-by: Kevin Anderson <[email protected]>
Co-authored-by: Kevin Anderson <[email protected]>
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.
A few more comments, and the old magnus_tetens.py
file still needs to be removed.
Otherwise I think we just need to tick off the remaining checklist items at the top of this thread: list the new functions in the reference docs, and create a what's new entry!
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.
A few more nitpicks, otherwise LGTM!
pvlib/tests/test_atmosphere.py
Outdated
# Unit tests | ||
def test_rh_from_tdew(): | ||
|
||
# dewpoint temp calculated with who and aekr coefficients |
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.
several comments and variables in these two test functions say "who" instead of "wmo" :)
pvlib/atmosphere.py
Outdated
@@ -337,6 +337,82 @@ def gueymard94_pw(temp_air, relative_humidity): | |||
return pw | |||
|
|||
|
|||
def rh_from_tdew(temperature, dewpoint, coeff=(6.112, 17.62, 243.12)): | |||
""" | |||
Calculate relative humidity from dewpoint temperature using the Magnus equation. |
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.
Calculate relative humidity from dewpoint temperature using the Magnus equation. | |
Calculate relative humidity from dew-point temperature using the Magnus equation. |
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.
(inner grammarian emerges) the term is either "dewpoint" or "dew point": NOAA glossary. The AMS glossary prefers "dewpoint".
I can't find any instances of "dew-point" as a hyphenated adjective, although I understand that's what grammar rules suggest it should be, rather than "dew point".
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.
Admittedly I didn't search beyond the WMO document we referenced, so it could be an old-world / new-world thing.
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.
Happy to use any term that you all prefer
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.
Looks like EPW and TMY3 files use the hyphen. Most of the pvlib docs use "dew point", with the exception of the Nomenclature page which says "Dewpoint".
I suggest we leave this for a potential package-wide cleanup and not worry about it here.
# First calculate ln(es/A) | ||
ln_term = ( | ||
(coeff[1] * temperature) / (coeff[2] + temperature) | ||
+ np.log(relative_humidity/100) |
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.
looks like RH zero might throw an error or warning here
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.
Should we add a check for zero values or leave the original message in the stack trace?
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 don't think it's necessary, RH=0% is practically impossible (source).
pvlib/atmosphere.py
Outdated
Returns | ||
------- | ||
numeric | ||
Dewpoint temperature in degrees Celsius |
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.
Dewpoint temperature in degrees Celsius | |
Dew-point temperature in degrees Celsius |
Co-authored-by: Anton Driesse <[email protected]>
Co-authored-by: Kevin Anderson <[email protected]>
Co-authored-by: Kevin Anderson <[email protected]>
Co-authored-by: Kevin Anderson <[email protected]>
Co-authored-by: Kevin Anderson <[email protected]>
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.
Does it make sense to add a test that demonstrates that you converting to rh and then back to tdew gives the same answer? If possible it would be a nice simple test.
Co-authored-by: Kevin Anderson <[email protected]>
Co-authored-by: Adam R. Jensen <[email protected]>
Co-authored-by: Adam R. Jensen <[email protected]>
Co-authored-by: Adam R. Jensen <[email protected]>
Co-authored-by: Adam R. Jensen <[email protected]>
Added a new test to show that you can calculate relative humidity from dewpoint and then calculate dewpoint from relative humidity and it will be the same as the original dewpoint values
Just added this test in the latest commit |
pvlib/atmosphere.py
Outdated
@@ -337,6 +337,86 @@ def gueymard94_pw(temp_air, relative_humidity): | |||
return pw | |||
|
|||
|
|||
def rh_from_tdew(temperature, dewpoint, coeff=(6.112, 17.62, 243.12)): |
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 noticed earlier but forgot to comment: we should rename this parameter to temp_dew
, the name used by other pvlib models and iotools functions. I'll push a commit for that to give @kurt-rhee a break from this PR :P
edit: and same for temp_air
of course
I think it's time to merge this. Thanks @kurt-rhee for the PR, @adriesse for putting us on the right course, and everyone else that contributed to the discussion! |
docs/sphinx/source/reference
for API changes.docs/sphinx/source/whatsnew
for all changes. Includes link to the GitHub Issue with:issue:`num`
or this Pull Request with:pull:`num`
. Includes contributor name and/or GitHub username (link with:ghuser:`user`
).remote-data
) and Milestone are assigned to the Pull Request and linked Issue.Reasoning
Caveats
Caveat 1
Caveat 2
pvlib.atmosphere.py
where the gueymard94 function exists. I'd like the maintainers opinion before proceeding to update the docs and what's new file.