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

Add declarative attribute plot_units #1314

Merged
merged 4 commits into from Feb 7, 2020

Conversation

kgoebber
Copy link
Collaborator

@kgoebber kgoebber commented Feb 2, 2020

Description Of Changes

This adds a new attribute, plot_units, to help with simple unit conversions for plotting. It is available for all Plot2D() subclasses including ContourPlot(), FilledContourPlot(), ImagePlot(), and BarbPlot().

Two new tests have been added to cover unit conversations for scalar and barb plots.

Running declarative tests fail likely due to running Matplotlib 3.1.2 locally and positions of text on some plots (especially those tests using PlotObs()) change slightly. Don't know if they will fail on Travis, so we'll see.

This is an immediate need for teaching class in just over a week in order to not have to over complicate making simple maps with our common units, which are not the stored SI units in our data files (e.g., Celsius instead of Kelvin and Knots instead of m/s).

Checklist

  • Tests added
  • Fully documented

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.

I can merge as-is if need be, but I think using the accessor for unit-handling would make the code much simpler.

data_subset = data.metpy.sel(**subset).squeeze()

if self.plot_units is not None:
data_subset.values = (data_subset.values
Copy link
Member

Choose a reason for hiding this comment

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

I think you should be able to do this as data_subset.metpy.convert_units(self.plot_units). That would eliminate the need to then set the unit attribute below I believe.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I concur, as this will prevent the need for an immediate refactor after #1304.

With the decision made in #1304, I would definitely recommend that the units of a DataArray should only be handled by the accessor (since the units will soon be part of the variable data inside of the DataArray rather than an attribute on the DataArray).

data_subset_v = v.metpy.sel(**subset).squeeze()

if self.plot_units is not None:
data_subset_u.values = (data_subset_u.values
Copy link
Member

Choose a reason for hiding this comment

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

Here too.

@kgoebber
Copy link
Collaborator Author

kgoebber commented Feb 7, 2020

Well that was a lot cleaner and more robust!

@dopplershift
Copy link
Member

Looks like some changes to underlying versions changed our coverage, but unrelated to your changes. I'll open an issue about coverage, but we can merge over.

@dopplershift dopplershift merged commit 40d5c12 into Unidata:master Feb 7, 2020
@dopplershift dopplershift added Area: Plots Pertains to producing plots Type: Enhancement Enhancement to existing functionality labels Feb 7, 2020
@dopplershift dopplershift added this to the 1.0 milestone Feb 7, 2020
@dopplershift
Copy link
Member

@kgoebber Was there a reason we didn't add such support to PlotObs?

@kgoebber
Copy link
Collaborator Author

kgoebber commented Feb 28, 2020 via email

@kgoebber
Copy link
Collaborator Author

So investigating this morning this would be a bit more involved on two fronts. First, there is not as easy of a conversion mechanism yet for non-vector observation quantities within StationPlot(), second, if reading in surface data from a csv file there will be no units attributes, so there will be a need to bake in some checks and raise errors altering when conversion can't happen as a result of no units as part of the dataframe.

Overall it shouldn't be too hard and I can probably get a PR done by the end of the day, but will involve modifying stationplot.py to make it easier to convert parameter units on the fly and then back that attribute into the declarative PlotObs().

Currently there is the ability to convert the units for vectors (both barbs and arrows) in StationPlot(), but not a parameter. The function doing the converting is _plotting_units(), so my initial thought is to change that to _vector_plotting_units() and craft a new _scalar_plotting_units()`.

@kgoebber
Copy link
Collaborator Author

kgoebber commented Mar 2, 2020

This PR partially resoled an issue in #1248 .

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
Development

Successfully merging this pull request may close these issues.

None yet

3 participants