Add test for GH 18523 and add _tz_compare() to compare timezone of DatetimeIndex #18596

MridulS · 2017-12-01T18:24:39Z

closes pd.concat reset the tz-aware index to UTC #18523
tests added / passed
passes git diff upstream/master -u -- "*.py" | flake8 --diff
whatsnew entry

Add a new private function _tz_compare which compares timezones of DatetimeIndex.

@jreback

…teTimeIndex

jreback

can you add a release not in bug fixes for 0.22.0

jreback · 2017-12-01T18:34:18Z

pandas/core/indexes/datetimes.py

@@ -1138,7 +1153,7 @@ def _maybe_utc_convert(self, other):
                raise TypeError('Cannot join tz-naive with tz-aware '
                                'DatetimeIndex')

-            if self.tz != other.tz:
+            if not self._tz_compare(other):


can you also add for all the other cases of str(self.tz) != str(other) (or variants on this)

The other places where pandas uses variants for str(self.tz) != str(other.tz) isn't particularly for DatetimeIndex. For example in test_timezones.py this is used to test the equality of Timestamp I guess we have to write a more general method which takes in any object with a .tz attribute and compare it. In that case which file should I put this method in?

the right place to put this is in
pandas/_libs/tslibs/timezones.pyx

and just call it from where needed

FYI - I added an additional instance of this in a commit that went in earlier today. Current location on master:

pandas/pandas/core/indexes/interval.py

Lines 242 to 243 in 288bf6e

elif (isinstance(left, ABCDatetimeIndex) and

str(left.tz) != str(right.tz)):

jreback · 2017-12-01T18:34:30Z

pandas/tests/indexes/datetimes/test_construction.py

@@ -443,6 +443,26 @@ def test_000constructor_resolution(self):

        assert idx.nanosecond[0] == t1.nanosecond

+    def test_concat(self):
+        idx1 = pd.date_range('2011-01-01', periods=3, freq='H',


pls add the issue number as a comment

jreback · 2017-12-01T18:34:45Z

pandas/tests/indexes/datetimes/test_construction.py

+                             tz='Europe/Paris')
+        idx2 = pd.date_range(start=idx1[0], end=idx1[-1], freq='H')
+        df1 = pd.DataFrame({'a': [1, 2, 3]}, index=idx1)
+        df2 = pd.DataFrame({'b': [1, 2, 3]}, index=idx2)


can you move this to pandas/tests/reshape/test_concat.py

jreback · 2017-12-01T18:35:10Z

pandas/tests/indexes/datetimes/test_construction.py

+        df3 = pd.DataFrame({'b': [1, 2, 3]}, index=idx3)
+        res = pd.concat([df1, df3], axis=1)
+
+        assert str(res.index.tzinfo) == 'UTC'


instead of this, can you construct the expected frame directly and use
tm.assert_frame_equal (same as above as well)

jreback · 2017-12-01T18:35:58Z

pandas/core/indexes/datetimes.py

@@ -1127,6 +1127,21 @@ def join(self, other, how='left', level=None, return_indexers=False,
        return Index.join(this, other, how=how, level=level,
                          return_indexers=return_indexers, sort=sort)

+    def _tz_compare(self, other):
+        """
+        Compare time zones of DatetimeIndex.


can you expound on the reason for this here (e.g. mention the directly comparing pytz of the same timezone is broken), but a str comparison is ok.

codecov · 2017-12-01T21:12:19Z

Codecov Report

Merging #18596 into master will decrease coverage by 0.04%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master   #18596      +/-   ##
==========================================
- Coverage   91.46%   91.41%   -0.05%     
==========================================
  Files         157      157              
  Lines       51452    51449       -3     
==========================================
- Hits        47059    47033      -26     
- Misses       4393     4416      +23

Flag	Coverage Δ
#multiple	`89.28% <100%> (-0.03%)`	⬇️
#single	`40.58% <100%> (-0.06%)`	⬇️

Impacted Files	Coverage Δ
pandas/core/indexes/datetimes.py	`95.6% <100%> (-0.11%)`	⬇️
pandas/io/gbq.py	`25% <0%> (-58.34%)`	⬇️
pandas/plotting/_converter.py	`63.44% <0%> (-1.82%)`	⬇️
pandas/core/indexes/multi.py	`96.27% <0%> (-0.17%)`	⬇️
pandas/core/frame.py	`97.81% <0%> (-0.1%)`	⬇️
pandas/core/base.py	`96.54% <0%> (-0.01%)`	⬇️
pandas/core/indexes/base.py	`96.46% <0%> (-0.01%)`	⬇️
pandas/core/series.py	`94.8% <0%> (+0.04%)`	⬆️
pandas/core/indexes/datetimelike.py	`97.11% <0%> (+0.19%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d5ffb1f...259d6d8. Read the comment docs.

codecov · 2017-12-01T21:12:32Z

Codecov Report

Merging #18596 into master will decrease coverage by 0.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master   #18596      +/-   ##
==========================================
- Coverage   91.46%   91.44%   -0.02%     
==========================================
  Files         157      157              
  Lines       51452    51449       -3     
==========================================
- Hits        47059    47048      -11     
- Misses       4393     4401       +8

Flag	Coverage Δ
#multiple	`89.31% <100%> (ø)`	⬆️
#single	`40.58% <100%> (-0.06%)`	⬇️

Impacted Files	Coverage Δ
pandas/core/indexes/datetimes.py	`95.7% <100%> (-0.02%)`	⬇️
pandas/io/gbq.py	`25% <0%> (-58.34%)`	⬇️
pandas/core/indexes/multi.py	`96.27% <0%> (-0.17%)`	⬇️
pandas/core/frame.py	`97.81% <0%> (-0.1%)`	⬇️
pandas/core/base.py	`96.54% <0%> (-0.01%)`	⬇️
pandas/core/indexes/base.py	`96.46% <0%> (-0.01%)`	⬇️
pandas/core/series.py	`94.8% <0%> (+0.04%)`	⬆️
pandas/util/testing.py	`81.89% <0%> (+0.19%)`	⬆️
pandas/core/indexes/datetimelike.py	`97.11% <0%> (+0.19%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d5ffb1f...259d6d8. Read the comment docs.

jbrockmendel

Could/should this be part of DatetimeIbdex._has_same_tz?

jreback · 2017-12-02T15:44:20Z

Could/should this be part of DatetimeIbdex._has_same_tz?

so looks there was an attempt to do this with _has_same_tz
can leave DatetimeIndex._has_same_tz as this has a conversion for a scalar as input. But let's remove _timezone, and replace _has_same_tz this call e.g. something like

    def _has_same_tz(self, other):

        if isinstance(other, (np.datetime64, date, datetime)):
            other = Timestamp(other)
        return timezones.tz_compare(self.tz, other.tz)

in tslibs/timezones.

cpdef tz_compare(object left, object, right):
    left = getattr(left, 'tz', '_no_tz')
    right = getattr(right, 'tz', '_no_tz')

     return get_timezone(left) == get_timezone(right)

and in most of the calls that right now are str(self.tz) == str(other.tz) can use _has_same_tz (maybe have to use tz_compare in some)

jreback · 2017-12-08T11:27:38Z

xref #18558

cc @jschendel

jreback · 2018-01-17T11:39:56Z

superseded by #19281

Add test for GH 18523 and add _tz_compare() to compare timezone of Da…

3436c78

…teTimeIndex

jreback requested changes Dec 1, 2017

View reviewed changes

jreback added Bug Reshaping Concat, Merge/Join, Stack/Unstack, Explode Timezones Timezone data dtype labels Dec 1, 2017

Address comments on the PR

259d6d8

jbrockmendel reviewed Dec 2, 2017

View reviewed changes

jreback mentioned this pull request Dec 23, 2017

ENH: support pendulum objects #15986

Closed

This was referenced Jan 17, 2018

Resample discards timezone information #13238

Closed

BUG: timezone comparisions are inconsistent, manifesting in bugs in .concat #19281

Merged

jreback closed this Feb 24, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add test for GH 18523 and add _tz_compare() to compare timezone of DatetimeIndex #18596

Add test for GH 18523 and add _tz_compare() to compare timezone of DatetimeIndex #18596

MridulS commented Dec 1, 2017 •

edited

Loading

jreback left a comment

jreback Dec 1, 2017

MridulS Dec 1, 2017

jreback Dec 1, 2017

jschendel Dec 9, 2017

jreback Dec 1, 2017

jreback Dec 1, 2017

jreback Dec 1, 2017

jreback Dec 1, 2017

codecov bot commented Dec 1, 2017 •

edited

Loading

codecov bot commented Dec 1, 2017 •

edited

Loading

jbrockmendel left a comment

jreback commented Dec 2, 2017

jreback commented Dec 8, 2017

jreback commented Jan 17, 2018

	elif (isinstance(left, ABCDatetimeIndex) and
	str(left.tz) != str(right.tz)):

Add test for GH 18523 and add _tz_compare() to compare timezone of DatetimeIndex #18596

Add test for GH 18523 and add _tz_compare() to compare timezone of DatetimeIndex #18596

Conversation

MridulS commented Dec 1, 2017 • edited Loading

jreback left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Dec 1, 2017 • edited Loading

Codecov Report

codecov bot commented Dec 1, 2017 • edited Loading

Codecov Report

jbrockmendel left a comment

Choose a reason for hiding this comment

jreback commented Dec 2, 2017

jreback commented Dec 8, 2017

jreback commented Jan 17, 2018

MridulS commented Dec 1, 2017 •

edited

Loading

codecov bot commented Dec 1, 2017 •

edited

Loading

codecov bot commented Dec 1, 2017 •

edited

Loading