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

SkewT Updates #1180

Merged
merged 10 commits into from Sep 27, 2019
Merged

SkewT Updates #1180

merged 10 commits into from Sep 27, 2019

Conversation

dopplershift
Copy link
Member

@dopplershift dopplershift commented Sep 26, 2019

Description Of Changes

  • Refactor SkewT to set more in the init method, including: log scaling, ticking, axis units, plot limits; this eliminates doing it as part of plot, which was weird
  • plot() now wraps matplotlib's plot() rather than semilogy().
  • We set a default aspect ratio that produces a good, standard-looking plot, finally fixing Fix aspect ratio for Skew-Ts #60. This will require matplotlib 3.2. This will work by adjusting the axes box by default, though this can be changed by the user (using matplotlib's set_adjustable) to tweak the data limits instead.

Checklist

@dopplershift dopplershift added Area: Plots Pertains to producing plots Type: Enhancement Enhancement to existing functionality labels Sep 26, 2019
@dopplershift dopplershift added this to the 0.11 milestone Sep 26, 2019
zbruick
zbruick previously approved these changes Sep 26, 2019
Copy link
Contributor

@zbruick zbruick left a comment

Choose a reason for hiding this comment

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

This will be a nice update to SkewT. Only question is is there a way to add an image test for the default aspect ratio with matplotlib >= 3.2. I know this would require adding the dev version of matplotlib to Travis. That's the only issue with codecov right now.

If you don't want to do this, then I think you have to merge, since I can't merge over required statuses.

@dopplershift
Copy link
Member Author

Probably should go ahead and add the test and mark it to skip on matplotlib < 3.2. I'll take a quick crack at updating with matplotlib git to see what that does.

@zbruick
Copy link
Contributor

zbruick commented Sep 26, 2019

Makes sense. At least having the test in there for when 3.2 is released would be worth it, even if we don't have it actually run on Travis right now.

@dopplershift
Copy link
Member Author

I'm gonna skip worrying about testing with matplotlib master right now, because the version on that (and on the v3.2.x branch) is 3.1.1.post2178+g9461967b9. There's no sensible way to make our version checking pass for that and not pass for future bugfix releases on the 3.1.x branch, which won't have the aspect support for log scaling.

@dopplershift
Copy link
Member Author

I've added the test, though it won't currently help with coverage until we can run something from 3.2. This test does capture the testing I've done to see how well we match the NWS SkewT diagram PDF--which at this point matches very well (once the proper rotation is set).

zbruick
zbruick previously approved these changes Sep 26, 2019
Copy link
Contributor

@zbruick zbruick left a comment

Choose a reason for hiding this comment

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

This looks good then, pending CI. You'll have to merge this one over codecov.

This has the impact of making a blank SkewT much more useful. Refactor
SkewT to set the log scaling, and appropriate ticking, at init time.
Also set default plot ranges to improve UX and avoid matplotlib problems
with log scaling + default limits + linear ticking (results in millions
of ticks).

This leaves plot() as a simple wrapper around matplotlib's plot now, and
it doesn't set any other state. That seems like a big improvement.
Now that we have our transform figured out better, we can leverage
matplotlib's support for locking down the aspect ratio (though this only
works with log scaling on Matplotlib >= 3.2). This adds an option to
control it and sets a default value that matches NWS plots.
This helps ensure that data values are correctly plotted against our
default plot ranges. it also makes sure that these units are always set
for other plot methods.
Specifying a default aspect ratio will break all these tests on
Matplotlib 3.2, so go ahead and manually revert back to the old
behavior for aspect.
Needed now that default/blank SkewT has more applied to it.
This will only really pass on matplotlib >= 3.2, so it sets a really
large tolerance on other versions so that the code at least executes.
This test also checks what a blank plot with no plots looks like.
@dopplershift
Copy link
Member Author

Well, I thought I was about to merge, but I accidentally left text on the test image for the aspect ratio. I've fixed that and went ahead and included my fix for #1176.

When we moved to doing the tickon, etc. equivalent at draw time, that
broke when saving figures with bbox_inches='tight', because that doesn't
go through draw(). Hook into the (private) method that determines the
tick bboxes.
Copy link
Contributor

@zbruick zbruick left a comment

Choose a reason for hiding this comment

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

Looks good!

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
None yet
2 participants