8000 API: Make datetime64 timezone naive by shoyer · Pull Request #6453 · numpy/numpy · GitHub
[go: up one dir, main page]

Skip to content

API: Make datetime64 timezone naive #6453

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 3 commits into from
Jan 17, 2016
Merged

Conversation

shoyer
Copy link
Member
@shoyer shoyer commented Oct 12, 2015

Fixes #3290

With apologies to @mwiebe, this rips out most of the time zone parsing from the datetime64 type.

I think we mostly sorted out the API design in discussions last year, but nonetheless I'll be posting this to the mailing list shortly to get feedback.

Old behavior:

# string parsing and printing defaults to your local timezone :(
>>> np.datetime64('2000-01-01T00')
numpy.datetime64('2000-01-01T00:00-0800','h')

New behavior:

# datetime64 is parsed and printed as timezone naive
>>> np.datetime64('2000-01-01T00')
numpy.datetime64('2000-01-01T00','h')

# you can still supply a timezone, but you get a deprecation warning
>>> np.datetime64('2000-01-01T00Z')
DeprecationWarning: parsing timezone aware datetimes is deprecated; this
will raise an error in the future
numpy.datetime64('2000-01-01T00','h')

Still be resolved: How should we handle the function np.datetime_as_string? As far as I can tell, it was never advertised in the public API and mostly exists for testing, but it still exists in the main numpy namespace :(. If we can remove it, then we can delete and simplify a lot more code related to timezone parsing and display. If not, we'll need to do a bit of work so we can distinguish between the string representations of timezone naive and UTC.

cc @PythonCHB @jreback

@shoyer shoyer force-pushed the naive-datetime64 branch 3 times, most recently from 4290f40 to 19ca170 Compare October 12, 2015 08:35
@shoyer shoyer mentioned this pull request Oct 12, 2015
class DatetimeFormat(object):
def __init__(self, x, unit=None,
timezone=None, casting='same_kind'):
def __init__(self, x, unit=None, timezone='naive', casting='same_kind'):
Copy link

Choose a reason for hiding this comment

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

not sure what the standard for numpy is, but usually timezone=None is by definition naive.

Choose a reason for hiding this comment

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

Agreed -- None should mean naive time.

Copy link
Member

Choose a reason for hiding this comment

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

The lack of a naive time zone was probably the biggest problem in the datetime NEP as it was when I started doing any datetime work. Unfortunately I didn't realize it should be added, and the discussion at the time didn't help with that.

Copy link
Member Author

Choose a reason for hiding this comment

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

OK, timezone=None now implies timezone='naive'

@jreback
Copy link
jreback commented Oct 12, 2015

lgtm from pandas point of view, fixes the 'display issue', which confounds users.

@PythonCHB
Copy link

@shoyer : Thank you SO MUCH for doing this -- it's been a long time!

@PythonCHB
Copy link

On Sun, Oct 11, 2015 at 11:51 PM, Stephan Hoyer notifications@github.com
wrote:

you can still supply a timezone, but you get a deprecation warning

np.datetime64('2000-01-01T00Z')
DeprecationWarning: parsing timezone aware datetimes is deprecated; this
will raise an error in the future
numpy.datetime64('2000-01-01T00','h')

This is a bit odd -- so it's kinda-sorta UTC? i.e if you supply an offset,
you get UTC? but then it's completely naive from there on out?

sigh -- I suppose we need to keep that for backwards compat. -- but I
wonder if the lack of any timezone on the other end will break it anyway?

Does anyone have use-cases, where they pass in offsets and need UTC? But
not any other TZ handling?

I'm a bit tempted to only allow "Z" or a 00:00 offset (though still
deprecate that), as that really makes no difference.

-CHB

Still be resolved: How should we handle the function np.datetime_to_string?
As far as I can tell, it was never advertised in the public API and mostly
exists for testing, but it still exists in the main numpy namespace :(. If
we can remove it, then we can delete and simplify a lot more code related
to timezone parsing and display. If not, we'll need to do a bit of work so
we can distinguish between timezone naive and UTC (which should be denoted
by ending with Z).

cc @PythonCHB https://github.com/PythonCHB @jreback

https://github.com/jreback

You can view, comment on, or merge this pull request online at:

#6453
Commit Summary

  • WIP: Make datetime64 timezone naive

File Changes

Patch Links:


Reply to this email directly or view it on GitHub
#6453.

Christopher Barker, PhD

Python Language Consulting

  • Teaching
  • Scientific Software Development
  • Desktop GUI and Web Development
  • wxPython, numpy, scipy, Cython

@shoyer
Copy link
Member Author
shoyer commented Oct 13, 2015

@PythonCHB Yes, this would preserve the legacy behavior of converting datetimes with explicitly provided timezones into UTC. I think this is the least painful way to transition from the current behavior, which is something like "UTC only, but prints in local time".

@PythonCHB
Copy link

On Tuesday, October 13, 2015, Stephan Hoyer notifications@github.com
wrote:

@PythonCHB https://github.com/PythonCHB Yes, this would preserve the
legacy behavior of converting datetimes with explicitly provided timezones
into UTC.

And probably necessary-- as I posted on the Numpy list -- with the current
code, you HAVE to specify UTC in an iOS string to not get any offset
applied.

-CHB

I think this is the least painful way to transition from the current
behavior, which is something like "UTC only, but prints in local time".


Reply to this email directly or view it on GitHub
#6453 (comment).

Christopher Barker, PhD

Python Language Consulting

  • Teaching
  • Scientific Software Development
  • Desktop GUI and Web Development
  • wxPython, numpy, scipy, Cython

@shoyer
Copy link
Member Author
shoyer commented Oct 14, 2015

OK, the latest patch preserves np.datetime_as_string (perhaps we should deprecate it, but I don't think there's cause to eliminate it immediately), though the default timezone treatment has shifted to naive.

@charris
Copy link
Member
charris commented Jan 12, 2016

@shoyer Could you add a description of this to the 1.11 release notes, squash the commits, fix up the commit messages. @mwiebe Mark, could you a quick look at this?

if (out_local != NULL) {
*out_local = 1;
*out_local = 0;
Copy link
Member

Choose a reason for hiding this comment

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

Might be better to rename the parameter from out_local to out_naive, keep it set to 1 here, and update the documentation comment for the function, etc.

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually, maybe we just entirely drop this parameter instead? AFAICT, it is not actually used anywhere.

@mwiebe
Copy link
Member
mwiebe commented Jan 12, 2016

The change looks reasonable to me. The way I would describe this is turning the datetime from "strongly typed" as UTC only (with bugs around the edges) into "weakly typed" via a naive time zone. Probably the main lesson from experience is that the datetime64 design should have included a naive timezone.

@@ -20,6 +21,14 @@
_has_pytz = False


@contextlib.contextmanager
def assert_deprecated():
Copy link
Member Author

Choose a reason for hiding this comment

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

do we have an existing context manager that does this check?

Copy link
Member

Choose a reason for hiding this comment

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

No, but this isn't correct, w is empty if the test fails and you will get an IndexError. We should fix up assert_warns so that it works. The problem (I think) is that DeprecationWarning and a few others are turned into errors for development testing. We usually do something like

    with warnings.catch_warnings():
        warnings.simplefilter('error', DeprecationWarning)
        assert_raises(DeprecationWarning, ...)

but that is clumsy. Let me take a look at assert_warns.

Copy link
Member

Choose a reason for hiding this comment

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

Oh, and the stack trace is somewhat extreme. We should fix that also.

Copy link
Member

Choose a reason for hiding this comment

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

OK, assert_warns should work in this case.

Copy link
Member

Choose a reason for hiding this comment

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

There is also a test_deprecations.py file that helps us track deprecations.

@shoyer shoyer force-pushed the naive-datetime64 branch 4 times, most recently from d997743 to f98e770 Compare January 13, 2016 06:10
@shoyer
Copy link
Member Author
shoyer commented Jan 13, 2016

The last commit includes fixes per @mwiebe's review, but also two other changes:

  • documentation for the release notes and updates to the datetime64 docs.
  • casting from dates to times is now allowed (e.g., np.datetime64('2000-01-01', 'h'), because the distinction is no longer relevant with time-zone naive datetimes.

@shoyer shoyer changed the title WIP: Make datetime64 timezone naive ENH: Make datetime64 timezone naive Jan 13, 2016
@shoyer shoyer changed the title ENH: Make datetime64 timezone naive API: Make datetime64 timezone naive Jan 13, 2016
@charris
Copy link
Member
charris commented Jan 14, 2016

@mwiebe Thanks for taking a look.

@shoyer
Copy link
Member Author
shoyer commented Jan 15, 2016

@charris oops -- looks like my rebase ate your comments. But I think I have addressed them.

Note: I extended assert_warns to work like a context manager. It's just very awkward to use it with a callable in code that actually does something.

@shoyer shoyer force-pushed the naive-datetime64 branch 2 times, most recently from e91d7f0 to e743657 Compare January 15, 2016 08:01
@@ -1,5 +1,6 @@
from __future__ import division, absolute_import, print_function

import contextlib
Copy link
Member

Choose a reason for hiding this comment

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

Don't need this any more.

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed

@charris
Copy link
Member
charris commented Jan 15, 2016

I like the context manager idea, that new option should be mentioned in the release notes and the documentation strings should mention that the context manager functionality is new in 1.11. There is also an unneeded import of contextlib. Apart from that, LGTM. We should probably add the context manager option to assert_raises at some point.

@charris
Copy link
Member
charris commented Jan 15, 2016

Oh, in the deprecation checks, please add a comment that the deprecation is new in 1.11, doing that helps us keep track of when things were deprecated so that they can be removed or changed to errors at some future time.

@shoyer
Copy link
Member Author
shoyer commented Jan 15, 2016

assert_raises actually already works as a context manager -- at least if you are using Python 2.7 or newer. I'm not quite sure why that is but we wrap nose for the functionality currently. In any case that should take care of itself when we drop 2.6 support -- which will be soon, I think?

On Fri, Jan 15, 2016 at 8:45 AM, Charles Harris notifications@github.com
wrote:

I like the context manager idea, that new option should be mentioned in the release notes and the documentation strings should mention that the context manager functionality is new in 1.11. There is also an unneeded import of contextlib. Apart from that, LGTM. We should probably add the context manager option to assert_raises at some point.

Reply to this email directly or view it on GitHub:
#6453 (comment)

@charris
Copy link
Member
charris commented Jan 15, 2016

We'll drop 2.6 right after the 1.11.x branch.

@shoyer
Copy link
Member Author
shoyer commented Jan 16, 2016

Do we really want to announce new features in NumPy's internal test utilities? If anyone is using assert_warns outside of numpy they are probably doing it wrong...

Fixes GH3290

With apologies to mwiebe, this rips out most of the time zone parsing from
the datetime64 type.

I think we mostly sorted out the API design in discussions last year, but
I'll be posting this to the mailing list shortly to get feedback.

Old behavior:

	# string parsing and printing defaults to your local timezone :(
    >>> np.datetime64('2000-01-01T00')
    numpy.datetime64('2000-01-01T00:00-0800','h')

New behavior:

    # datetime64 is parsed and printed as timezone naive
    >>> np.datetime64('2000-01-01T00')
    numpy.datetime64('2000-01-01T00','h')

    # you can still supply a timezone, but you get a deprecation warning
    >>> np.datetime64('2000-01-01T00Z')
    DeprecationWarning: parsing timezone aware datetimes is deprecated; this
    will raise an error in the future
    numpy.datetime64('2000-01-01T00','h')
@shoyer
Copy link
Member Author
shoyer commented Jan 16, 2016

@charris I already have a note in the docstring for TestDatetime64Timezone that it is deprecated in 1.11.

@charris
Copy link
Member
charris commented Jan 16, 2016

They aren't numpy internal ;) If you grep scipy there are lots of uses and I expect there are other projects using them as well.

@shoyer
Copy link
Member Author
shoyer commented Jan 16, 2016

They aren't numpy internal ;)

Only because NumPy doesn't really have such a thing as private API ;). But fair enough.

@shoyer
Copy link
Member Author
shoyer commented Jan 17, 2016

Just pushed release notes and version changed documentation for assert_warns.

charris added a commit that referenced this pull request Jan 17, 2016
API: Make datetime64 timezone naive
@charris charris merged commit 865c3e3 into numpy:master Jan 17, 2016
@charris
Copy link
Member
charris commented Jan 17, 2016

Thanks Stephan.

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

Successfully merging this pull request may close these issues.

Timezones and Datetime64
5 participants
0