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

Fix for skewt subroutines ignoring x axis units #1216

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

jibbals
Copy link

@jibbals jibbals commented Oct 24, 2019

Wet and dry adiabats, along with mixing lines, do not correctly
plot unless everything is in degrees celcius. This is especially
a problem when the plots show the wrong values without giving
any sort of warning. This fix makes the subroutines plot onto
units matching the xaxis or input temperature.
Checklist

Closes #1155
test added

Description Of Changes

Checklist

  • Closes #xxxx
  • Tests added
  • Fully documented

@jibbals
Copy link
Author

jibbals commented Oct 24, 2019

This follows on from the closed unimplemented pull request #1168

@dopplershift dopplershift added this to the 0.12 milestone Oct 24, 2019
@zbruick zbruick added Area: Plots Pertains to producing plots Type: Enhancement Enhancement to existing functionality labels Oct 24, 2019
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.

A couple minor things, but I think this is really close.

@jibbals
Copy link
Author

jibbals commented Oct 29, 2019

When I run the pytest --mpl -k test_skewt.py, there is a warning.
I don't know what this warning is or if it applies to my changes?

TestWarning

@dopplershift
Copy link
Member

Let me look locally. Some of the other most recent builds don't show this, so I'm guessing something that's tweaked in this PR is causing it.

@dopplershift
Copy link
Member

Ok, so the error is caused by the fact that we're now doing a little bit of computing, so our this line:

ref_pressure < pressure.max()

ends up returning true, even though both are nominally 1000. Mind if I upload a commit to fix?

@dopplershift
Copy link
Member

Actually, on second thought I'll do a separate PR for that issue, since I'll need some tests for that. We can ignore the warning in your PR.

@dopplershift
Copy link
Member

You should be able to add the blank line and do:

git commit —amend
git push —force

@dopplershift
Copy link
Member

As far as the other tests, I can take a look and see what’s going on.

@jibbals
Copy link
Author

jibbals commented Oct 31, 2019

Thanks @dopplershift.

@dopplershift dopplershift modified the milestones: 0.12, 1.0 Dec 24, 2019
@dopplershift dopplershift modified the milestones: 1.0, 1.1 Jan 13, 2020
Base automatically changed from master to main February 22, 2021 22:39
@dopplershift dopplershift modified the milestones: 1.1.0, 1.2.0 Aug 2, 2021
@jibbals jibbals requested a review from a team as a code owner December 3, 2021 20:36
@dopplershift
Copy link
Member

dopplershift commented Dec 3, 2021

@jibbals Apologies on this languishing for so long. I took the liberty of rebasing this on current main and making sure everything works with how the code is now structured. I'm expecting this to pass now.

Regarding the warnings from lsoda, I've figured them out completely, it's pretty dumb. Essentially:

units.Quantity(1000., 'mbar') - units.Quantity(1000., 'hPa')

gives
-2.2737367544323206e-13 millibar rather than 0 thanks to the wonder that is floating point arithmetic.

@dopplershift dopplershift dismissed their stale review December 3, 2021 20:42

I've now added to this PR.

Wet and dry adiabats, along with mixing lines, do not correctly
plot unless everything is in degrees celcius. This is especially
a problem when the plots show the wrong values without giving
any sort of warning. This fix makes the subroutines plot onto
units matching the xaxis or input temperature.
Checklist

Closes Unidata#1155
test added
jibbals and others added 3 commits December 3, 2021 13:46
Reset pmin to be 600 hPa instead of 60pct of max pressure
Added test to hit edge case where user specifies kelvin t0
One fixes a unit error, another bring the spacing of moist adiabats back
in line with what we had before, where it was 10 below freezing, and 5
above.
@dcamron
Copy link
Member

dcamron commented Jan 21, 2022

So with cf734e3 deciding default units for the xaxis, and underlying calculations (e.g. dry_lapse) having unit handling since, the original behavior in #1155 no longer happens, and these tests (with units specified, even if mixed) pass as-is on 1.1 and on current main. Passing un-united arrays as t0 to e.g. plot_dry_adiabats does currently fail with a unit error per the underlying calculation. I'm fine with that. As far as I can tell, the only thing we gain here right now is the hidden t > 150 assumption, which does avoid unit errors, but (imo) incorrectly so. We do need some of this more flexible axis unit handling in the future, so let's leave this open but punt for now. Thanks all!

@dcamron dcamron modified the milestones: 1.2.0, 1.3.0 Jan 21, 2022
@dopplershift dopplershift modified the milestones: 1.3.0, May 2022 Apr 6, 2022
@dopplershift dopplershift modified the milestones: May 2022, July 2022 Jun 1, 2022
@dopplershift dopplershift removed this from the October 2022 milestone Oct 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: Plots Pertains to producing plots Type: Enhancement Enhancement to existing functionality
Projects
Status: In Progress
Development

Successfully merging this pull request may close these issues.

skewT subplots assume celcius without a warning
4 participants