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

fix mixing lines keyword args bug #2326

Merged
merged 10 commits into from
Sep 30, 2022
Merged

fix mixing lines keyword args bug #2326

merged 10 commits into from
Sep 30, 2022

Conversation

AndrewLoeppky
Copy link
Contributor

Description Of Changes

Hello! Im an atmospheric science grad student at UBC. I found + squished a bug in the skewt mixing lines function -- if you specify both pressure and mixing_ratio as not the defualts it raises an error. solution is to transpose the input mixing_ratio array as with the defualt.

Critiques/comments welcome, this is my first edit/pr to a code base that isnt mine...

Checklist

  • Closes #xxxx
  • Tests added
  • Fully documented

@AndrewLoeppky AndrewLoeppky requested a review from a team as a code owner February 2, 2022 13:30
@AndrewLoeppky AndrewLoeppky requested review from dcamron and removed request for a team February 2, 2022 13:30
@CLAassistant
Copy link

CLAassistant commented Feb 2, 2022 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

dopplershift
dopplershift reviewed Feb 2, 2022
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.

Good catch and thanks for contributing a fix! I have a minor suggestion on implementation. Also, would you be willing to add a test in tests/plots/test_skewt.py so that we don't accidentally break this in the future?

src/metpy/plots/skewt.py Outdated Show resolved Hide resolved
@dopplershift dopplershift added this to the 1.3.0 milestone Feb 2, 2022
@AndrewLoeppky @dopplershift
Update tests/plots/test_skewt.py
3ac4adc 
right... whoops thanks

Co-authored-by: Ryan May <rmay31@gmail.com>
@dopplershift dopplershift modified the milestones: 1.3.0, May 2022 Apr 6, 2022
@dopplershift dopplershift added Type: Bug Something is not working like it should Area: Plots Pertains to producing plots labels May 16, 2022
@dopplershift dopplershift modified the milestones: May 2022, July 2022 Jun 1, 2022
@dopplershift dopplershift reopened this Sep 30, 2022
@dopplershift dopplershift enabled auto-merge (squash) September 30, 2022 07:08
dopplershift
dopplershift approved these changes Sep 30, 2022
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.

Sorry this languished so long. Looks like the last missing piece was adding the reference image for the test (which I copied from the identical image from the other test).

Assuming the tests pass, this is ready to go! Thanks again for putting in the fix.

@dopplershift dopplershift merged commit c8d6767 into Unidata:main Sep 30, 2022
@dopplershift
Copy link
Member

Congratulations on your first merge PR to MetPy! 🎉 We hope we see you around more in the future!

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 dcamron is a code owner automatically assigned from Unidata/metpy-maintainers

Assignees
No one assigned
Labels
Area: Plots Pertains to producing plots Type: Bug Something is not working like it should
Projects
Python
Status: Done
Milestone
1.4.0
Development

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