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

cftime.datetime does not allow addition #198

Closed
mcgibbon opened this issue Aug 25, 2020 · 7 comments
Closed

cftime.datetime does not allow addition #198

mcgibbon opened this issue Aug 25, 2020 · 7 comments

Comments

@mcgibbon
Copy link
Contributor

cftime.datetime returns an object of type cftime._cftime.datetime, which cannot be added to or subtracted from using a timedelta. Based on its calendar keyword argument, I would expect it to return a subclass such as DatetimeJulian which has arithmetic defined.

Python 3.7.4 (default, Aug 13 2019, 15:17:50)
[Clang 4.0.1 (tags/RELEASE_401/final)] :: Anaconda, Inc. on darwin
Type "help", "copyright", "credits" or "license" for more information.
>>> import cftime
>>> dt = cftime.datetime(2020, 1, 1, calendar="julian")
>>> from datetime import timedelta
>>> dt += timedelta(hours=1)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: unsupported operand type(s) for +=: 'cftime._cftime.datetime' and 'datetime.timedelta'
>>> cftime.__version__
'1.2.1'
@spencerkclark
Copy link
Collaborator

spencerkclark commented Aug 25, 2020

Based on its calendar keyword argument, I would expect it to return a subclass such as DatetimeJulian which has arithmetic defined.

I would be +1 on this. It's often quite useful to be able to construct datetimes starting from a string calendar name rather than an explicit cftime type, which extending the constructor of cftime.datetime in this way would enable. E.g. in lieu of this we need to use logic like this in xarray.

@jswhit
Copy link
Collaborator

jswhit commented Aug 26, 2020

Pull request would be welcomed.

@jswhit
Copy link
Collaborator

jswhit commented Aug 27, 2020

PR #199 implements this by changing the name of the base class datetime to datetime_base and making datetime a factory function that returns calendar-specific datetime instances.

@jswhit
Copy link
Collaborator

jswhit commented Sep 25, 2020

PR #201 is an alternate fix that does not require changing the name of the base class. It does require changing the default calendar from "standard" to None. Since much of the code is moved from cython to pure python for this solution, it is quite a bit slower. More optimization is needed before it can be merged.

@jswhit
Copy link
Collaborator

jswhit commented Sep 28, 2020

PR #202 is yet another alternate fix that does not require changing the name of the base class, nor does it degrade performance by migrating the base class to python. It creates 'calendar-aware' instances of the base-class directly using the calendar kwarg to __init__ and a bunch if if/else statements in __add__ and __sub__. The sub-classes are still there, although they are just stubs that no longer over-ride any methods of the base class. This should work fine as long as no one is checking for the subclass type directly (instead of just checking the value of the calendar attribute).

jswhit added a commit that referenced this issue Sep 28, 2020
@jswhit
Copy link
Collaborator

jswhit commented Sep 29, 2020

xarray tests pass with PR #202. nc-time-axis requires a small change to the tests.

PR #202 is my preferred solution to this issue. It doesn't sacrifice any performance, and probably will have the least impact on existing code.

@jswhit
Copy link
Collaborator

jswhit commented Oct 27, 2020

closed by PR #202

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

3 participants