Fix index for datetime64 conversion. Fixes #13937 #14446

amolkahat · 2016-10-18T12:38:41Z

closes BUG: DataFrame.to_records() bug in converting datetime64 index with timezone #13937
tests added / passed
passes git diff upstream/master | flake8 --diff
0.20.0

Fix index for datetime64 conversion

This PR fixes index for datetime64 conversion. Also,
update tests - pandas/pandas/tests/tests_convert_to.py

codecov-io · 2016-10-19T07:26:50Z

Codecov Report

Merging #14446 into master will decrease coverage by -0.03%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master   #14446      +/-   ##
==========================================
- Coverage   91.06%   91.04%   -0.03%     
==========================================
  Files         136      136              
  Lines       49104    49104              
==========================================
- Hits        44718    44705      -13     
- Misses       4386     4399      +13

Impacted Files	Coverage Δ
pandas/core/frame.py	`97.74% <100%> (-0.1%)`	❌
pandas/io/gbq.py	`40% <0%> (-46.67%)`	❌
pandas/util/decorators.py	`67.92% <0%> (-0.95%)`	❌
pandas/tools/merge.py	`91.78% <0%> (-0.35%)`	❌
pandas/core/common.py	`91.02% <0%> (-0.34%)`	❌

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 29d81f3...3806983. Read the comment docs.

amolkahat · 2016-10-19T08:50:06Z

@jreback Please take a look. I updated tests as you suggested in PR #14044.

jreback · 2016-10-19T11:55:39Z

pandas/tests/frame/test_convert_to.py

            .to_records()
        expected = np.rec.array([('x', 'y')], dtype=[('a', 'O'), ('b', 'O')])
        tm.assert_almost_equal(result, expected)
+
+    def test_to_records_with_tz_gmt(self):


can you add a comment for the issue number

jreback · 2016-10-19T11:56:36Z

pandas/tests/frame/test_convert_to.py

+        df = DataFrame({'datetime': data})
+        df.set_index('datetime', inplace=True)
+        dfgmt = df.tz_convert("GMT")
+        tm.assertIsNot(df.to_records()['datetime'][0].tzinfo,


construct the expected frame and use assert_frrame_equal

Can you also simplify the construction of the test data using pandas constructs?

Eg

data = [datetime.datetime.now(pytz.timezone('UTC')) for _ in range(10)] df = DataFrame({'datetime': data}) df.set_index('datetime', inplace=True)

can be done with (for similar object, not exact same values of course)

df = DataFrame(index=date_range('2016-01-01', periods=10, freq='S', tz='UTC'))

jreback · 2016-10-19T11:56:59Z

pls add a whatsnew entry for 0.19.1

jorisvandenbossche · 2016-10-27T13:13:00Z

pandas/tests/frame/test_convert_to.py

+    def test_to_records_with_tz_gmt(self):
+        """
+        This test verifies the bug id #13937
+        """


Can you add this as a comment instead of docstring? (docstrings hide the test name in the output of nosetests)

Also just the number is fine, like # GH 13937

Ok. I'll take care of this. :)

jorisvandenbossche · 2016-10-27T13:18:05Z

pandas/tests/frame/test_convert_to.py

+        df3 = DataFrame({'datetime': dt3})
+        df4 = df3.set_index('datetime')
+
+        tm.assert_frame_equal(df2, df4)


Is this test related to the bug you fixed?
It does not use to_records

@jorisvandenbossche Hmm. yes it is not related to my bug fix. I just focused on data frame equality. Actually there is also one problem assert_frame_equal does not accept any other frames expect DataFrame. In my previous PR 14044 Jeff suggest to construct equal data frame. Can you suggest me how i complete Jeff's assert_frame_equal requirement?

@jorisvandenbossche @jreback Can you please suggest me how i construct equal data frame?

Your question is not fully clear to me.
If you want to test to_records, you can also assert that the resulting array is equal (with assert_numpy_array_equal instead of assert_frame_equal)

jorisvandenbossche · 2016-10-27T13:18:46Z

pandas/tests/frame/test_convert_to.py

+        df4 = DataFrame({'datetime': dt2})
+        df5 = df4.set_index('datetime')
+
+        tm.assert_frame_equal(df3, df5)


Same question for this test.
Was the above code failing before your PR? Or is this not yet tested?

No this code does not falling before my PR or after the fixes.

jorisvandenbossche · 2016-10-27T13:21:55Z

pandas/tests/frame/test_convert_to.py

+        df = DataFrame({'datetime': data})
+        df.set_index('datetime', inplace=True)
+        dfgmt = df.tz_convert("GMT")
+        tm.assertIsNot(df.to_records()['datetime'][0].tzinfo,


Can you also simplify the construction of the test data using pandas constructs?

Eg

data = [datetime.datetime.now(pytz.timezone('UTC')) for _ in range(10)] df = DataFrame({'datetime': data}) df.set_index('datetime', inplace=True)

can be done with (for similar object, not exact same values of course)

df = DataFrame(index=date_range('2016-01-01', periods=10, freq='S', tz='UTC'))

jorisvandenbossche · 2016-11-26T09:24:42Z

@amolkahat It looks like something went wrong with rebasing/updating. Normally, doing exactly the following should clean it up:

git fetch upstream
git checkout bug_fixes
git rebase upstream/master
git push origin bug_fixes -f

(supposing your fork is 'origin' and pandas-dev is 'upstream')

jorisvandenbossche · 2016-12-14T15:19:35Z

@amolkahat Can you rebase? (see my previous comment how to fix it)

amolkahat · 2016-12-30T07:55:52Z

@jorisvandenbossche Please take a look.

jreback · 2016-12-30T19:16:38Z

can you add a whatsnew note for 0.20.0

jreback · 2016-12-30T19:17:46Z

pandas/tests/frame/test_convert_to.py

+                                        freq='S',
+                                        tz='UTC'))
+
+        dfgmt = df.tz_convert("GMT")


use result = ....

can you construct a rec-array directly and use tm.assert_numpy_array_equal

jreback · 2016-12-30T19:19:04Z

pandas/tests/frame/test_convert_to.py

+    def test_from_UTC_to_GMT_as_equal_frame(self):
+        # GH13937
+
+        dt1 = [datetime.datetime(2011,


this last test is not necessary

jreback · 2016-12-30T19:19:37Z

pandas/tests/frame/test_convert_to.py

+        df1 = DataFrame(index=dt1)
+        df3 = df1.tz_convert("UTC")
+
+        df4 = DataFrame(index=dt2)


however the empty frame is good (but instead of a separate tests, you can do this as part of the other one, IOW)
just use a starting from from the first test

jreback

try to consolidate these tests into a single test (but with multiple parts is ok), its just confusing to start with 3 different frames, start with 1, then do a

result = ....
expected = ....
tm.assert_numpy_array_equal(....)

# next part
....

# last part
...

jreback

not sure if you updated this. the tests needs to be more concise and focused.

jreback · 2017-01-09T18:10:37Z

doc/source/whatsnew/v0.20.0.txt

@@ -355,3 +355,7 @@ Bug Fixes


 - Bug in ``pd.read_csv()`` for the C engine where ``usecols`` were being indexed incorrectly with ``parse_dates`` (:issue:`14792`)
+=======
+- Bug in ``DataFrame.to_records()`` with converting datetime64 index with timezone (:issue: `13937`)


so looks like some merge conflicts are remaining here

jreback · 2017-01-09T18:11:24Z

pandas/tests/frame/test_convert_to.py

+                                   8,
+                                   15,
+                                   8,
+                                   15,


this is really hard to read. please format better.

jreback · 2017-01-09T18:12:33Z

pandas/tests/frame/test_convert_to.py

+                                            freq='S',
+                                            tz='GMT'))
+
+        tm.assert_almost_equal(df_utc.to_records()['datetime'][0],


what does this comparison add that the assert_numpy_array_equal does not? (which is the way we would like to compare these)

jreback · 2017-01-09T18:13:10Z

pandas/tests/frame/test_convert_to.py

+                            index=data2)
+
+        df_gmt2 = DataFrame(date_range('2016-01-01',
+                                       periods=10,


what is this test actually testing? (which the first test does not)

jreback · 2017-01-14T18:21:38Z

can you update

amolkahat · 2017-02-19T09:22:08Z

@jreback Please take a look.

jreback · 2017-02-20T14:16:59Z

doc/source/whatsnew/v0.20.0.txt

@@ -589,3 +589,5 @@ Bug Fixes
 - Bug in ``Series.replace`` and ``DataFrame.replace`` which failed on empty replacement dicts (:issue:`15289`)
 - Bug in ``pd.melt()`` where passing a tuple value for ``value_vars`` caused a ``TypeError`` (:issue:`15348`)
 - Bug in ``.eval()`` which caused multiline evals to fail with local variables not on the first line (:issue:`15342`)
+


move into a blank space above so that you don't get conflicts.

with converting a datetime64 index

jreback · 2017-02-20T14:17:15Z

pandas/tests/frame/test_convert_to.py

 from pandas.tests.frame.common import TestData


 class TestDataFrameConvertTo(tm.TestCase, TestData):
+    _multiprocess_can_split_ = True


remove as we are using pytest now

jreback · 2017-02-20T14:22:10Z

pandas/tests/frame/test_convert_to.py

+                                            periods=10,
+                                            freq='S',
+                                            tz='UTC'))
+


do something simpler and much more readable

result = df_utc.tz_convert("GMT").to_records() expected = df_utc.to_records() tm.assert_numpy_array_equal(result, expected)

jreback · 2017-02-20T14:22:29Z

pandas/tests/frame/test_convert_to.py

+
+    def test_to_records_with_tz(self):
+        # GH13937
+        df_utc = DataFrame({'datetime': date_range('2016-01-01',


can you format this to make it easier to read

jreback · 2017-02-20T14:22:47Z

pandas/tests/frame/test_convert_to.py

+
+        # test DataFrame.to_records() with timezone conversion to UTC
+
+        df_gmt = DataFrame({'datetime': date_range('2016-01-01',


same with all of the following, make shorter / simpler / easier-to-read

jreback · 2017-02-28T19:03:33Z

@amolkahat thanks it looks a bit better. In general, please don't modify whitespace. You must have some editor / settings which are doing things. They are pretty distracting.

jreback · 2017-03-01T19:23:43Z

you'll want to rebase on master as travis was having some issues

Fix index for datetime64 conversion This PR fixes index for datetime64 conversion. Also, update tests - pandas/pandas/tests/tests_convert_to.py Fixes #13937

jreback · 2017-03-02T13:45:11Z

thanks!

closes pandas-dev#13937 Author: Amol Kahat <[email protected]> Closes pandas-dev#14446 from amolkahat/bug_fixes and squashes the following commits: 3806983 [Amol Kahat] Modify test cases.

amolkahat · 2017-03-03T08:13:57Z

@jreback Thanks.
Can you tell me on which IRC server & channel pandas-dev discussion happen?

jorisvandenbossche · 2017-03-03T09:59:50Z

@amolkahat for chat, there is the gitter chat (https://gitter.im/pydata/pandas), there is also the pandas-dev mailing list (https://mail.python.org/mailman/listinfo/pandas-dev), but most of the discussion happens on github

amolkahat · 2017-03-03T10:07:39Z

@jorisvandenbossche Thank you. 😃

closes pandas-dev#13937 Author: Amol Kahat <[email protected]> Closes pandas-dev#14446 from amolkahat/bug_fixes and squashes the following commits: 3806983 [Amol Kahat] Modify test cases.

jreback added Bug Reshaping Concat, Merge/Join, Stack/Unstack, Explode Timezones Timezone data dtype labels Oct 19, 2016

jreback reviewed Oct 19, 2016

View reviewed changes

jorisvandenbossche reviewed Oct 27, 2016

View reviewed changes

jreback reviewed Dec 30, 2016

View reviewed changes

jreback requested changes Dec 30, 2016

View reviewed changes

jreback requested changes Jan 9, 2017

View reviewed changes

jreback requested changes Feb 20, 2017

View reviewed changes

Modify test cases.

3806983

Fix index for datetime64 conversion This PR fixes index for datetime64 conversion. Also, update tests - pandas/pandas/tests/tests_convert_to.py Fixes #13937

jreback closed this in f000a4e Mar 2, 2017

jreback added this to the 0.20.0 milestone Mar 2, 2017

+,
+,
+,
+,


		# test DataFrame.to_records() with timezone conversion to UTC

		df_gmt = DataFrame({'datetime': date_range('2016-01-01',

Fix index for datetime64 conversion. Fixes #13937 #14446

Fix index for datetime64 conversion. Fixes #13937 #14446

Conversation

amolkahat commented Oct 18, 2016 • edited Loading

codecov-io commented Oct 19, 2016 • edited Loading

Codecov Report

amolkahat commented Oct 19, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jreback commented Oct 19, 2016

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

jorisvandenbossche commented Nov 26, 2016

jorisvandenbossche commented Dec 14, 2016

amolkahat commented Dec 30, 2016

jreback commented Dec 30, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jreback left a comment

Choose a reason for hiding this comment

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

jreback commented Jan 14, 2017

amolkahat commented Feb 19, 2017

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

jreback commented Feb 28, 2017

jreback commented Mar 1, 2017

jreback commented Mar 2, 2017

amolkahat commented Mar 3, 2017

jorisvandenbossche commented Mar 3, 2017

amolkahat commented Mar 3, 2017

amolkahat commented Oct 18, 2016 •

edited

Loading

codecov-io commented Oct 19, 2016 •

edited

Loading