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.

Sign up for GitHub

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

Jump to bottom

Add K Index calculate function. #1990

Merged
merged 10 commits into from
Aug 9, 2021
Merged

Add K Index calculate function. #1990

merged 10 commits into from
Aug 9, 2021

Conversation

C2oWisComing
Copy link
Contributor

@C2oWisComing C2oWisComing commented Jul 28, 2021 •
edited by dopplershift
Loading

Description Of Changes

Add a function for K Index calculation based on this formula:
K = (T850 - T500) + Td850 - (T700 - Td700)

This is my first pull request, Thank you for good python lib for weather research.

Checklist

@CLAassistant
Copy link

CLAassistant commented Jul 28, 2021 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

dopplershift
dopplershift requested changes Aug 2, 2021
View reviewed changes
Copy link
Member

@dopplershift dopplershift left a comment

Choose a reason for hiding this comment

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

Greetings and thanks for the contribution! This looks in really good shape with good docs and a test. I have a few comments about the implementation that will hopefully make it robust on all the various data sources we encounter. Also, can you make sure the blank lines here (especially in the docstring) do not contain any spaces/tabs? The style bots are complaining about some trailing whitespace.

src/metpy/calc/thermo.py Outdated Show resolved Hide resolved
src/metpy/calc/thermo.py Outdated Show resolved Hide resolved
src/metpy/calc/thermo.py Outdated Show resolved Hide resolved
tests/calc/test_thermo.py Outdated Show resolved Hide resolved
src/metpy/calc/thermo.py Outdated Show resolved Hide resolved
@dopplershift
Copy link
Member

This also looks like it's now in conflict with some other change we must have made to thermo.py.

@C2oWisComing
Copy link
Contributor Author

Thank you for your kind advice. I will fix it asap.

@C2oWisComing
- Add George1960 to docs/api/references.rst
13ea51e 
- Fix typo: "substract" -> "subtract" in src/metpy/calc/thermo.py
- Use interpolate_1d function to get the data for T and Td
- Fix list alphabetized in tests/calc/test_thermo.py
- Add blank line in docstring
@C2oWisComing
- Fix continuation line over-indented for visual indent.
0616fbf
@C2oWisComing
Fix Expected 2 blank lines after k_index function
f4f723f
@C2oWisComing
Copy link
Contributor Author

I'm sorry for my delay. I've already fixed it.

dopplershift
dopplershift requested changes Aug 9, 2021
View reviewed changes
Copy link
Member

@dopplershift dopplershift left a comment

Choose a reason for hiding this comment

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

Thanks, this is just about there. There were a couple minor things. Also, can you add k_index under the Soundings heading (in the right spot alphabetically) in docs/_templates/overrides/metpy.calc.rst? That will help our checks pass and make sure it shows up in our table of calculations.

src/metpy/calc/thermo.py Outdated Show resolved Hide resolved
src/metpy/calc/thermo.py Outdated Show resolved Hide resolved
@dopplershift dopplershift added Area: Calc Pertains to calculations Type: Feature New functionality labels Aug 9, 2021
@dopplershift dopplershift added this to the 1.2.0 milestone Aug 9, 2021
@C2oWisComing
- Change all variables to start with lowercase.
43a6c19 
- Add underscore at the end of reference point.
- Remove blank line after the docstring.
@dopplershift
Copy link
Member

FYI, this currently conflicts with test_thermo.py on the main branch.

@C2oWisComing
Copy link
Contributor Author

Thank you so much for your help. I hope it will be successful.

dopplershift
dopplershift requested changes Aug 9, 2021
View reviewed changes
Copy link
Member

@dopplershift dopplershift left a comment

Choose a reason for hiding this comment

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

Just a minor nit with a long line and this should pass all the checks.

src/metpy/calc/thermo.py Outdated Show resolved Hide resolved
@C2oWisComing
Copy link
Contributor Author

I already warp it.

@dopplershift dopplershift merged commit 79abf12 into Unidata:main Aug 9, 2021
@dopplershift
Copy link
Member

Congratulations on your first contribution to MetPy! 🎉 Thanks for putting this together!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dopplershift dopplershift dopplershift approved these changes

@dcamron dcamron Awaiting requested review from dcamron

Assignees
No one assigned
Labels
Area: Calc Pertains to calculations Type: Feature New functionality
Projects
None yet
Milestone
1.2.0
Development

Successfully merging this pull request may close these issues.
3 participants
@C2oWisComing @CLAassistant @dopplershift