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

Implement matplotlib converters? #247

Closed
aulemahal opened this issue Jun 3, 2021 · 10 comments
Closed

Implement matplotlib converters? #247

aulemahal opened this issue Jun 3, 2021 · 10 comments

Comments

@aulemahal
Copy link

aulemahal commented Jun 3, 2021

Since the 1.5.0 release, plotting cftimes with matplotlib is broken because nc-time-axis uses deprecated parts of cftime. A PR was open to fix this issue (SciTools/nc-time-axis#59), but it hasn't seen any activity yet. In fact, that package is not very active in general (which is understandable as it is quite small).

I was wondering if it would make sense to implement its functionality directly here, in cftime? Matplotlib would not be added as a dependency, but when present the converters could be registered upon import? The code seems low maintenance in general and with a compatible licence, but moving it here would reduce the number situations like right now.

I am willing to contribute to this.

What do @jbusecke, @trexfeathers and @jswhit think?

@jbusecke
Copy link

jbusecke commented Jun 3, 2021

I honestly do not understand these inner workings enough and as such would rely on the opinion of others here. Thanks for the heads up though.

@jswhit
Copy link
Collaborator

jswhit commented Jun 3, 2021

I know nothing about how nc-time-axis works either. If it's a simple matter of converting to a matplotlib standard calendar, we could provide that here - but providing tickers and formatters should be left in a separate package I think. Seems like the fundamental problem is that nc-time-axis is not very actively maintained.

@trexfeathers
Copy link

Seems like the fundamental problem is that nc-time-axis is not very actively maintained.

I agree. We create many good ideas in the SciTools organisation without knowing if we have the resource/priority to maintain them. I will continue to keep my colleagues up to date on these problems, so we can try to find time to sort them out.

@aulemahal
Copy link
Author

@jswhit Sadly, I don't think converting to a standard calendar would work as expected, all those non-standard dates would get in the way. The nc-time-axis package is pretty much 3 classes : a Ticker, a Locator and a Converter. The latter converts to integers (rather than to standard datetimes).

I believe it is not actively maintained in part because it is relatively low maintenance : the matplotlib interface seems quite stable.

@jswhit
Copy link
Collaborator

jswhit commented Jun 4, 2021

@aulemahal so you are proposing moving the Converter from nc-time-axis here (without adding a matplotlib dependency)? From a cursory look at the code it seems that it won't be that simple (the Converter uses the Locator and the Formatter).

@aulemahal
Copy link
Author

Oh no, sorry. previous comment was a bit ambiguous.
I am suggesting moving the whole thing to cftime. Without adding a matplotlib dependency indeed. Idea: put this in a "mpl_utils.py" and, in __init__.py do:

try: 
    import matplotlib
except ImportError:
    pass
else:
    import mpl_utils

@jswhit
Copy link
Collaborator

jswhit commented Jun 4, 2021

I would only want to do this at the request of the nc-time-axis team - it seems to me that there are a lot of good reasons to keep it separate. @trexfeathers what do you think?

@trexfeathers
Copy link

@trexfeathers what do you think?

I'm going to spend some time this week trying to sort out nc-time-axis, cf-units, and any other dependent packages we have that are struggling to keep pace.

@bjlittle
Copy link
Collaborator

bjlittle commented Jun 7, 2021

Ping @aulemahal @jbusecke

Fancy joining the discussion over on SciTools/nc-time-axis ? 😄

@jswhit
Copy link
Collaborator

jswhit commented Jun 8, 2021

sounds like nc-time-axis is getting jumpstarted - which means we shouldn't throw a monkey wrench in that effort by merging nc-time-axis functionality into cftime. Going to close this now.

@jswhit jswhit closed this as completed Jun 8, 2021
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

No branches or pull requests

5 participants