-
-
Notifications
You must be signed in to change notification settings - Fork 10.9k
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
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.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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'
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.
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.
There was a problem hiding this comment.
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.
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.
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?
There was a problem hiding this comment.
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
.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
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