Skip to content

BUG: Correctly localize naive datetime strings with Series and datetimetztype (#17415) #17603

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.

Sign up for GitHub

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 6 commits into from
Jan 12, 2018
Merged

BUG: Correctly localize naive datetime strings with Series and datetimetztype (#17415) #17603

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
0f3be92
BUG: Correctly localize naive ts strings with Series and datetimetz d…
mroeschke Sep 20, 2017
735f35b
Add missing arg
mroeschke Dec 11, 2017
18e914b
fix failing test and rebase
mroeschke Dec 12, 2017
bb10828
Correct to a more appropriate test
mroeschke Dec 13, 2017
e66397d
Rebase and move whatsnew to v0.23
mroeschke Jan 11, 2018
cee514f
address comments
mroeschke Jan 12, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion doc/source/whatsnew/v0.23.0.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -380,7 +380,7 @@ Conversion
- Fixed bug where comparing :class:`DatetimeIndex` failed to raise ``TypeError`` when attempting to compare timezone-aware and timezone-naive datetimelike objects (:issue:`18162`)
- Bug in :class:`DatetimeIndex` where the repr was not showing high-precision time values at the end of a day (e.g., 23:59:59.999999999) (:issue:`19030`)
- Bug where dividing a scalar timedelta-like object with :class:`TimedeltaIndex` performed the reciprocal operation (:issue:`19125`)
-
- Bug in localization of a naive, datetime string in a ``Series`` constructor with a ``datetime[ns, timezone]`` dtype (:issue:`174151`)
Copy link
Contributor

Choose a reason for hiding this comment

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

datetime64[ns, tz]


Indexing
^^^^^^^^
Expand Down
20 changes: 13 additions & 7 deletions pandas/core/dtypes/cast.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@
is_integer_dtype,
is_datetime_or_timedelta_dtype,
is_bool_dtype, is_scalar,
_string_dtypes,
is_string_dtype, _string_dtypes,
pandas_dtype,
_ensure_int8, _ensure_int16,
_ensure_int32, _ensure_int64,
Expand Down Expand Up @@ -1003,12 +1003,18 @@ def maybe_cast_to_datetime(value, dtype, errors='raise'):
if is_datetime64:
value = to_datetime(value, errors=errors)._values
elif is_datetime64tz:
# input has to be UTC at this point, so just
# localize
value = (to_datetime(value, errors=errors)
.tz_localize('UTC')
.tz_convert(dtype.tz)
)
# This block can be simplified once PR #17413 is
Copy link
Contributor

Choose a reason for hiding this comment

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

add to the comment what that PR is doing

Copy link
Contributor

Choose a reason for hiding this comment

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

add an xref (with this PR) to that issue as well

Copy link
Contributor

Choose a reason for hiding this comment

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

change to #13712 here (the issue and not the PR which was closed)

# complete
is_dt_string = is_string_dtype(value)
value = to_datetime(value, errors=errors)
if is_dt_string:
Copy link
Contributor

Choose a reason for hiding this comment

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

hmm adding this incremental logic is error prone. I think this should simply be converted higher up. can you see if this can be simplied that way.

Copy link
Member Author

Choose a reason for hiding this comment

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

This logic is pretty high up the stack already.

The Series constructor calls _sanitize_array which then calls maybe_cast_to_datetime (which I am editing here) fairly early. The line I am editing is the first instance where arrays of numbers (and strings) are converted with to_datetime

Copy link
Contributor

Choose a reason for hiding this comment

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

try passing utc=True to to_datetime

this logic is too specific here

Copy link
Contributor

Choose a reason for hiding this comment

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

actually it might be that

to_datetime(...., tz=dtype.tz) might be the correct idiom here (another PR implements this)

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah #17413 will make this easier.

However I think there will need to be logic to determine if the incoming data is naive or already localized to UTC. The code comments that were already here note that (numeric) data is already UTC while string data (what I am trying to fix here) may be naive.

Copy link
Contributor

Choose a reason for hiding this comment

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

my point is solving this here is wrong

this is a bug in to_datetime itself

Copy link
Contributor

Choose a reason for hiding this comment

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

passing utc=True to to_datetime will make this work (then just converting to the dtype)

Copy link
Contributor

Choose a reason for hiding this comment

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

can you try passing utc=True here

Copy link
Member Author

Choose a reason for hiding this comment

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

utc=True does not work here because the timestamp string should be localized to the specified dtype tz directly intead of UTC in between.

# with utc=True
In [5]: ts_naive = '2013-01-01 00:00:00'
   ...:  result = Series([ts_naive], dtype='datetime64[ns, CET]')
   ...:  expected = Series([pd.Timestamp(ts_naive, tz='CET')])

In [6]: result
Out[6]: 
0   2013-01-01 01:00:00+01:00
dtype: datetime64[ns, CET]

In [7]: expected
Out[7]: 
0   2013-01-01 00:00:00+01:00
dtype: datetime64[ns, CET]

# Strings here are naive, so directly localize
value = value.tz_localize(dtype.tz)
else:
# Numeric values are UTC at this point,
# so localize and convert
value = (value.tz_localize('UTC')
.tz_convert(dtype.tz))
elif is_timedelta64:
value = to_timedelta(value, errors=errors)._values
except (AttributeError, ValueError, TypeError):
Expand Down
8 changes: 8 additions & 0 deletions pandas/tests/series/test_constructors.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -707,6 +707,14 @@ def test_constructor_with_datetime_tz(self):
expected = Series(pd.DatetimeIndex(['NaT', 'NaT'], tz='US/Eastern'))
assert_series_equal(s, expected)

@pytest.mark.parametrize('arg',
['2013-01-01 00:00:00', pd.NaT, np.nan, None])
def test_constructor_with_naive_string_and_datetimetz_dtype(self, arg):
# GH 17415: With naive string
result = Series([arg], dtype='datetime64[ns, CET]')
expected = Series(pd.Timestamp(arg)).dt.tz_localize('CET')
assert_series_equal(result, expected)

def test_construction_interval(self):
# construction from interval & array of intervals
index = IntervalIndex.from_breaks(np.arange(3), closed='right')
Expand Down