test_dti_tz_localize_roundtrip fails locally

[robbuckley]

Code Sample, a copy-pastable example if possible

pandas.test(['-k', 'test_dti_tz_localize_roundtrip'])

pandas/tests/indexes/datetimes/test_timezones.py ...........................F............                                                                     [100%]

============================================================================= FAILURES ==============================================================================
_________________________________________ TestDatetimeIndexTimezones.test_dti_tz_localize_roundtrip[tz_aware_fixture6-idx3] _________________________________________

self = <pandas.tests.indexes.datetimes.test_timezones.TestDatetimeIndexTimezones object at 0x1c27c88f28>, tz_aware_fixture = tzlocal()
idx = DatetimeIndex(['2014-08-01 00:00:00', '2014-08-01 00:01:00',
               '2014-08-01 00:02:00', '2014-08-01 00:03:0...          '2014-10-30 23:59:00', '2014-10-31 00:00:00'],
              dtype='datetime64[ns]', length=131041, freq='T')

    @pytest.mark.parametrize('idx', [
        date_range(start='2014-01-01', end='2014-12-31', freq='M'),
        date_range(start='2014-01-01', end='2014-12-31', freq='D'),
        date_range(start='2014-01-01', end='2014-03-01', freq='H'),
        date_range(start='2014-08-01', end='2014-10-31', freq='T')
    ])
    def test_dti_tz_localize_roundtrip(self, tz_aware_fixture, idx):
        tz = tz_aware_fixture
        localized = idx.tz_localize(tz)
        expected = date_range(start=idx[0], end=idx[-1], freq=idx.freq,
                              tz=tz)
>       tm.assert_index_equal(localized, expected)
E       AssertionError: Index are different
E       
E       Index length are different
E       [left]:  131041, DatetimeIndex(['2014-08-01 00:00:00+01:00', '2014-08-01 00:01:00+01:00',
E                      '2014-08-01 00:02:00+01:00', '2014-08-01 00:03:00+01:00',
E                      '2014-08-01 00:04:00+01:00', '2014-08-01 00:05:00+01:00',
E                      '2014-08-01 00:06:00+01:00', '2014-08-01 00:07:00+01:00',
E                      '2014-08-01 00:08:00+01:00', '2014-08-01 00:09:00+01:00',
E                      ...
E                      '2014-10-30 23:51:00+00:00', '2014-10-30 23:52:00+00:00',
E                      '2014-10-30 23:53:00+00:00', '2014-10-30 23:54:00+00:00',
E                      '2014-10-30 23:55:00+00:00', '2014-10-30 23:56:00+00:00',
E                      '2014-10-30 23:57:00+00:00', '2014-10-30 23:58:00+00:00',
E                      '2014-10-30 23:59:00+00:00', '2014-10-31 00:00:00+00:00'],
E                     dtype='datetime64[ns, tzlocal()]', length=131041, freq='T')
E       [right]: 131101, DatetimeIndex(['2014-08-01 00:00:00+01:00', '2014-08-01 00:01:00+01:00',
E                      '2014-08-01 00:02:00+01:00', '2014-08-01 00:03:00+01:00',
E                      '2014-08-01 00:04:00+01:00', '2014-08-01 00:05:00+01:00',
E                      '2014-08-01 00:06:00+01:00', '2014-08-01 00:07:00+01:00',
E                      '2014-08-01 00:08:00+01:00', '2014-08-01 00:09:00+01:00',
E                      ...
E                      '2014-10-30 23:51:00+00:00', '2014-10-30 23:52:00+00:00',
E                      '2014-10-30 23:53:00+00:00', '2014-10-30 23:54:00+00:00',
E                      '2014-10-30 23:55:00+00:00', '2014-10-30 23:56:00+00:00',
E                      '2014-10-30 23:57:00+00:00', '2014-10-30 23:58:00+00:00',
E                      '2014-10-30 23:59:00+00:00', '2014-10-31 00:00:00+00:00'],
E                     dtype='datetime64[ns, tzlocal()]', length=131101, freq='T')

pandas/tests/indexes/datetimes/test_timezones.py:448: AssertionError

Problem description

Fails locally but passes in CI on the same commit. Its because the test fixture includes tzlocal(), and for some timezones (incl most of europe), one of the testcases spans a DST transition:

date_range(start='2014-08-01', end='2014-10-31', freq='T')

Expected Output

should pass, regardless of the local timezone

Output of pd.show_versions()

INSTALLED VERSIONS

commit: 7c842b0
python: 3.7.2.final.0
python-bits: 64
OS: Darwin
OS-release: 18.2.0
machine: x86_64
processor: i386
byteorder: little
LC_ALL: None
LANG: en_GB.UTF-8
LOCALE: en_GB.UTF-8

pandas: 0.24.0.dev0+1539.g7c842b031
pytest: 4.0.2
pip: 18.1
setuptools: 40.6.2
Cython: 0.29.2
numpy: 1.15.4
scipy: 1.1.0
pyarrow: None
xarray: None
IPython: 7.2.0
sphinx: 1.8.2
patsy: 0.5.1
dateutil: 2.7.5
pytz: 2018.7
blosc: None
bottleneck: 1.2.1
tables: 3.4.4
numexpr: 2.6.8
feather: None
matplotlib: 3.0.2
openpyxl: 2.5.12
xlrd: 1.2.0
xlwt: 1.3.0
xlsxwriter: 1.1.2
lxml.etree: 4.2.5
bs4: 4.6.3
html5lib: 1.0.1
sqlalchemy: 1.2.15
pymysql: 0.9.2
psycopg2: None
jinja2: 2.10
s3fs: 0.2.0
fastparquet: None
pandas_gbq: None
pandas_datareader: None
gcsfs: 0.2.0

[gfyoung]

@robbuckley : That's a good catch. Perhaps we could monkey-patch tzlocal to return in different timezones to test that case (don't think we need to change locale entirely for this).

PR to address this would be welcomed!

cc @mroeschke

[mroeschke]

I'd be ideal to test tzlocal in various locales to catch errors like this. A couple of things:

1. What is your system's local timezone?
2. Does this test still pass if you replace tzlocal with dateutil/your_timezone?

[robbuckley]

my local timezone is GMT/BST. If I add 'Europe/London' to the test fixture, tz_localize(tz) fails with pytz.exceptions.AmbiguousTimeError. I can get around that by changing it to tz_localize(tz, ambiguous='NaT'), then if fails in a very similar way to using tzlocal()

I have a proposed patch to the testcase, that addresses this issue. I will submit a PR

[robbuckley]

@mroeschke not sure what you mean by 'test tzlocal in various locales': (1) overriding the locale in travis or (2) adding timezones to the test fixture?

[mroeschke]

What I meant is akin to overriding the locale in the CI (1), but @gfyoung's suggestion may be better to just mock tzlocal instead (2). Overriding in the CI may be too heavy handed to only benefit a small subset of our test suite.

[robbuckley]

OK, thanks. isn't mocking tzlocal just equivalent to adding timezones to the fixture? I have a change with that running now, let me know what you think

[mroeschke]

Essentially there's two things:

1. Handling tzlocal objects explicitly in the code base. They take different paths compared to pytz or other dateutil timezone objects.
2. Equivalent to what you're mentioning, evaluating tzlocal as if it contained the same offset information as GMT/BST or US/Pacific or etc...

[mroeschke]

Feel free to send a PR with the test change @robbuckley. It will be easier to review that way.

[robbuckley]

@mroeschke i just pushed the PR. I didnt mock tzlocal yet, i will have a think about that. Meanwhile i added a TZ to the fixture which provokes the failure, and rewrote the TC to address that case

[robbuckley]

@mroeschke thinking further on this: there are 4 special cases to deal with when localizing a range of tz-naive datetimes as in this TC, assuming there's just one DST transition in the range.

1. STD>DST transition, non-tzlocal() case
2. STD>DST transition, tzlocal() case
3. DST>STD transition, non-tzlocal() case
4. DST>STD transition, tzlocal() case

1 and 2 may have non-existent times (valid as neither STD nor DST)
3 and 4 may have ambiguous times (valid as both)

For example for Europe/London (GMT / BST) in 2014:

• times from 01:00 to just before 02:00 on 30-mar don't exist in either time
• times from 01:00 until 02:00 on 26-oct exist in both times

tz_localize() seems to handle these 2 cases differently for tzlocal() and other timezones, even if they have the same DST transition points. For example on my system (with default values of the tz_localize() params ambiguous and nonexistent):

1. raise NonExistentTimeError
2. nonexistent times are moved back 1 hour and localized to STD time, producing duplicate times(!)
3. raise AmbiguousTimeError
4. ambiguous times are interpreted as DST, similar to tz_localize(tz, ambiguous=False)

The existing version of the testcase doesnt cover any of these special cases. Cases 1 and 2 because the combination of non-tzlocal() timezones in tz_aware_fixture and time intervals in the testcase dont include any DST transitions. Cases 3 and 4 because tzlocal() is UTC on travis.

My proposal is to use the existing PR to extend test_dti_tz_localize_roundtrip to cover cases 1 and 3, but remove the tzlocal() case, and to submit a 2nd PR for the tzlocal() scenarios including cases 3 and 4 with a new testcase -- call it test_dti_tz_localize_roundtrip_tzlocal.

This 2nd PR would need some kind of mocking of tzlocal() as you suggested, in order to get adequate coverage and remove the dependence on the local tz. This could be tricky - i had a quick look at the dateutil code to see how they test tzlocal(), it looks like they havent mocked this either.

if you're OK with this course of action, i'll have another go at it.

[mroeschke]

Hmm so taking a step back, I think the main purpose of this test is to test this basic behavior:

1. Create a naive date range
2. Attach a timezone to the date range
3. Remove the time zone
4. Assert the result is equal to the beginning

idx = date_range(...)
localized = idx.tz_localize(tz)
reset = localized.tz_localize(None)
tm.assert_index_equal(reset, idx)

Therefore when a DST transition exists, I would not expect this to roundtrip because attaching the timezone is problematic which you described (and is ok).

So while I appreciate the thought behind how to reorganize the test to catch these different cases, I suggest just simplifying this test all together by testing a date range where we're guaranteed not to have an ambiguous or nonexistent time (e.g. date_range('2018-05-04 05:00:00', periods=3, freq='s'), just guessing).

[robbuckley]

hi, that makes sense, thanks. I've pushed a new version of the tc which avoids the DST transitions. Maybe there's a case for adding some back in future in another tc, if they would add any meaningful coverage (which im not sure they would).