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

Handle Pandas time indices within StationPlots #1130

Merged
merged 2 commits into from Aug 23, 2019

Conversation

zbruick
Copy link
Contributor

@zbruick zbruick commented Aug 19, 2019

Description Of Changes

Adds support for Pandas DatetimeIndex and TimedeltaIndex as valid x variables in StationPlot. This addresses #957, which shows a use case for a meteogram of sky cover (and weather symbols could be imagined too). If needed, other datetime types could be added for further support.

This also adds a dependency on Pandas, which I think we're now up to 3 PRs in the 0.11 queue with this dependency.

Checklist

@zbruick zbruick added Type: Bug Something is not working like it should Area: Plots Pertains to producing plots labels Aug 19, 2019
@zbruick zbruick added this to the 0.11 milestone Aug 19, 2019
setup.py Outdated Show resolved Hide resolved
@zbruick
Copy link
Contributor Author

zbruick commented Aug 21, 2019

Got the unit handling figured out I think. I added an image test (based on #957), but since this is my first to do, please let me know if I need to change anything. I dropped all of the tolerances down to 0 and it passed on my machine, so I'm curious how CI does with it, or if the test isn't doing what I think it is.

@zbruick zbruick force-pushed the stationplot_pandas branch 3 times, most recently from 3a7bc05 to a43fa2b Compare August 21, 2019 20:23
@dopplershift
Copy link
Member

Do we need to add the Pandas dependency here if we're not actually using it?

@dopplershift
Copy link
Member

Oh, it's used in testing...

@zbruick
Copy link
Contributor Author

zbruick commented Aug 21, 2019

We already have it as a testing dependency I believe (#1034). So I think it could be removed from this PR.

@jthielen
Copy link
Collaborator

jthielen commented Aug 21, 2019

I might be missing something, but isn't pandas already a dependency of MetPy, since pandas is a hard dependency of xarray? It would just be determining which versions to support/test?

@dopplershift
Copy link
Member

@zbruick You might cherry-pick the commits onto another of your PRs to save the work. We might ship 0.11 without the other PRs (who knows) so to me it seems better to only add it as a strict dependency when we actually have it.

@jthielen While the environment will have Pandas because of Xarray, we should only be listing dependencies on things that MetPy actually imports in the library proper. That way if for some reason xarray drops its dependence on Pandas, we're not forcing it needlessly in our packaging. Probably not a practical difference here, but a good policy to follow.

Likewise, we should have a listing for everything we import and not just rely on it being pulled in transitively from something else we depend on.

@zbruick
Copy link
Contributor Author

zbruick commented Aug 21, 2019

Yeah I agree with the comments here. No reason to add it in this PR - I forgot I was dropping what I was using it for with the update. Good suggestion to cherrypick them over before dropping them. Fine with me to ship 0.11 without the DSG PR, as I'm guessing the appending stuff I added may take some time to review and revise.

Resolves issues with Pandas DatetimeIndex.
@zbruick zbruick force-pushed the stationplot_pandas branch 2 times, most recently from 6e1ab33 to 116d522 Compare August 22, 2019 15:19
@zbruick
Copy link
Contributor Author

zbruick commented Aug 22, 2019

Pandas dependency has been removed, and this PR has been rebased.

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 tweak to this test and I think it's ready to go.

metpy/plots/tests/test_station_plot.py Outdated Show resolved Hide resolved
@dopplershift dopplershift merged commit 74eaab3 into Unidata:master Aug 23, 2019
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: Bug Something is not working like it should
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Metpy StationPlot does not like dates
3 participants