Skip to content

Add support for plotting subclasses of cftime.datetime #42

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

Merged
merged 1 commit into from
Jan 25, 2019

Conversation

spencerkclark
Copy link
Member

We would love to use nc-time-axis in xarray. There was some discussion over the summer about possibly adding support for plotting subclasses of cftime.datetime (e.g. cftime.DatetimeNoLeap) directly. This would make using it in xarray somewhat easier. This PR is an attempt to add that. In so doing, I've retained support for plotting nc_time_axis.CalendarDateTime objects, so this should be backwards-compatible.

Let me know if there is still interest in this. I'm happy to iterate on it if desired.

For example, with this PR one can now do something like this:

import random

import matplotlib.pyplot as plt
import nc_time_axis
import cftime

d_time = [cftime.Datetime360Day(year=2017, month=2, day=n) for n in range(1, 31)]
temperatures = [round(random.uniform(0, 12), 3) for _ in range(len(d_time))]

plt.plot(d_time, temperatures)
plt.margins(0.1)
plt.ylim(0, 12)
plt.xlabel("Date")
plt.ylabel("Temperature")
plt.show()

image

@coveralls
Copy link

Coverage Status

Coverage increased (+0.7%) to 91.228% when pulling 0ab0395 on spencerkclark:raw-cftime into 46aee1a on SciTools:master.

@jhamman
Copy link

jhamman commented Jan 10, 2019

ping @pelson and @DPeterK for a review.

@DPeterK DPeterK requested a review from lbdreyer January 11, 2019 10:50
@spencerkclark
Copy link
Member Author

@lbdreyer when you get a chance please let me know if you would be interested in this. Thanks!

If this seems like something you would rather wait to think about, we'll probably go forward with using CalendarDateTime in xarray for now (pydata/xarray#2665, cc: @jbusecke).

Copy link
Member

@lbdreyer lbdreyer left a comment

Choose a reason for hiding this comment

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

Thanks for this @spencerkclark !

This looks like a nice clean addition that's ready for merging.

There is one more thing we need from you before I can merge this. Could you fill in this CLA form https://scitools.org.uk/cla/v4/form?

Once that's done we can merge this in and then we can cut a new release of nc-time-axis so that you are unblocked!

@spencerkclark
Copy link
Member Author

Once that's done we can merge this in and then we can cut a new release of nc-time-axis so that you are unblocked!

Awesome, @lbdreyer, thank you! I just signed the CLA. I really appreciate your work on this package.

@lbdreyer
Copy link
Member

I just signed the CLA

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants