API: Make datetime64 timezone naive #6453
Conversation
4290f40 to
19ca170
Compare
| 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'): |
There was a problem hiding this comment.
not sure what the standard for numpy is, but usually timezone=None is by definition naive.
There was a problem hiding this comment.
Agreed -- None should mean naive time.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
OK, timezone=None now implies timezone='naive'
|
lgtm from pandas point of view, fixes the 'display issue', which confounds users. |
|
@shoyer : Thank you SO MUCH for doing this -- it's been a long time! |
|
On Sun, Oct 11, 2015 at 11:51 PM, Stephan Hoyer notifications@github.com
sigh -- I suppose we need to keep that for backwards compat. -- but I Does anyone have use-cases, where they pass in offsets and need UTC? But I'm a bit tempted to only allow "Z" or a 00:00 offset (though still -CHB
Christopher Barker, PhD Python Language Consulting
|
|
@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". |
|
On Tuesday, October 13, 2015, Stephan Hoyer notifications@github.com
-CHB
Christopher Barker, PhD Python Language Consulting
|
|
OK, the latest patch preserves |
| if (out_local != NULL) { | ||
| *out_local = 1; | ||
| *out_local = 0; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Actually, maybe we just entirely drop this parameter instead? AFAICT, it is not actually used anywhere.
|
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(): | |||
There was a problem hiding this comment.
do we have an existing context manager that does this check?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Oh, and the stack trace is somewhat extreme. We should fix that also.
There was a problem hiding this comment.
OK, assert_warns should work in this case.
There was a problem hiding this comment.
There is also a test_deprecations.py file that helps us track deprecations.
d997743 to
f98e770
Compare
|
The last commit includes fixes per @mwiebe's review, but also two other changes:
|
|
@mwiebe Thanks for taking a look. |
0404545 to
d588b48
Compare
|
@charris oops -- looks like my rebase ate your comments. But I think I have addressed them. Note: I extended |
e91d7f0 to
e743657
Compare
| @@ -1,5 +1,6 @@ | |||
| from __future__ import division, absolute_import, print_function | |||
|
|
|||
| import contextlib | |||
|
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 |
|
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. |
|
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
|
|
We'll drop 2.6 right after the 1.11.x branch. |
|
Do we really want to announce new features in NumPy's internal test utilities? If anyone is using |
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')
e743657 to
4261148
Compare
|
@charris I already have a note in the docstring for |
|
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. |
Only because NumPy doesn't really have such a thing as private API ;). But fair enough. |
|
Just pushed release notes and version changed documentation for |
API: Make datetime64 timezone naive
|
Thanks Stephan. |
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:
New behavior:
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