DEPR: convert_datetime64 parameter in to_records() #18902

reidy-p · 2017-12-21T23:44:27Z

closes DataFrame.to_records() converts DatetimeIndex to datetime object, not np.datetime64 #18160
tests added / passed
passes git diff upstream/master -u -- "*.py" | flake8 --diff
whatsnew entry

As noted in the original issue, the underlying NumPy bug seems to be fixed so this parameter may no longer be needed.

I also changed the default value for convert_datetime64 from True to False and only give a FutureWarning when convert_datetime64=True is passed but I'm not sure if this is more appropriate than leaving True as the default and always giving a FutureWarning.

jschendel · 2017-12-21T23:48:35Z

doc/source/whatsnew/v0.23.0.txt

@@ -216,6 +216,7 @@ Deprecations
 - ``Series.valid`` is deprecated. Use :meth:`Series.dropna` instead (:issue:`18800`).
 - :func:`read_excel` has deprecated the ``skip_footer`` parameter. Use ``skipfooter`` instead (:issue:`18836`)
 - The ``is_copy`` attribute is deprecated and will be removed in a future version (:issue:`18801`).
+- The ``convert_datetime64`` parameter has been deprecated and the default value is now ``False`` in :func:`to_records` as the NumPy bug motivating this parameter has been resolved (:issue:`18160`).


Should :func:`to_records` actually be :func:`DataFrame.to_records`?

@jschendel : I believe you are correct

@reidy-p : This sentence is a run-on. Break it up into two sentences (and add comma's at the conjunctions).

gfyoung · 2017-12-22T02:27:31Z

@reidy-p : I'm a little surprised to see that you didn't have to change any tests with this deprecation, as it is also an API change. If this passes, it means coverage for this function is not that great.

Perhaps we should take the opportunity to write a few more tests...

reidy-p · 2017-12-22T09:31:01Z

@gfyoung yeah I was surprised too. I'll take a look at the existing tests in more detail and see if I can add some more.

codecov · 2017-12-22T09:39:22Z

Codecov Report

Merging #18902 into master will decrease coverage by 0.02%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master   #18902      +/-   ##
==========================================
- Coverage   91.84%   91.81%   -0.03%     
==========================================
  Files         153      153              
  Lines       49275    49277       +2     
==========================================
- Hits        45255    45245      -10     
- Misses       4020     4032      +12

Flag	Coverage Δ
#multiple	`90.2% <100%> (-0.03%)`	⬇️
#single	`41.91% <33.33%> (-0.01%)`	⬇️

Impacted Files	Coverage Δ
pandas/core/frame.py	`97.16% <100%> (ø)`	⬆️
pandas/plotting/_converter.py	`65.07% <0%> (-1.74%)`	⬇️

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 3885ced...4a1b1ba. Read the comment docs.

jreback · 2017-12-23T20:58:44Z

pandas/core/frame.py

@@ -1187,7 +1187,7 @@ def from_records(cls, data, index=None, exclude=None, columns=None,

        return cls(mgr)

-    def to_records(self, index=True, convert_datetime64=True):
+    def to_records(self, index=True, convert_datetime64=False):


make the default value None.

@reidy-p did you see my comment here

Yeah, I changed the default to None but was then asked to change it to False instead in a later review.

I think there are at least two options for handling the deprecation of the convert_datetime64 parameter:

Change default from True to None. Give a warning if convert_datetime64 is not None. If convert_datetime64 is None then set convert_datetime64=True inside the function to avoid breaking existing code.

Change default from True to False. Give a warning if convert_datetime64=True.

Which option is better?

@jorisvandenbossche I think you recommended Option 1.

yes I think we need to make this None, then can detect that a user is passing this. Yes we are also completely changing the behavior of this, but we really want to remove this parameter.

This is in-line with @jorisvandenbossche comments. We want to warn if a user is passing this at ALL, and we are changing the default. (so its now False if the parameter is not specified). can you reword as appropriate. ping when ready.

jreback · 2017-12-23T20:59:03Z

pandas/core/frame.py

@@ -1196,14 +1196,23 @@ def to_records(self, index=True, convert_datetime64=True):
        ----------
        index : boolean, default True
            Include index in resulting record array, stored in 'index' field
-        convert_datetime64 : boolean, default True
+        convert_datetime64 : boolean, default False


boolean, optional

jreback · 2017-12-23T20:59:24Z

pandas/core/frame.py

            Whether to convert the index to datetime.datetime if it is a
            DatetimeIndex

        Returns
        -------
        y : recarray
        """
+
+        if convert_datetime64:


is not None. We are going to completely remove this parameter, so we want to warn if its passed at all.

jreback · 2017-12-23T20:59:43Z

pandas/tests/frame/test_convert_to.py

+        with tm.assert_produces_warning(FutureWarning):
+            expected = df.index[0]
+            result = df.to_records(convert_datetime64=True)['index'][0]
+            assert expected == result

        rs = df.to_records(convert_datetime64=False)


you will need to catch the warning on the following as well

jreback · 2017-12-23T21:00:01Z

doc/source/whatsnew/v0.23.0.txt

@@ -216,6 +216,7 @@ Deprecations
 - ``Series.valid`` is deprecated. Use :meth:`Series.dropna` instead (:issue:`18800`).
 - :func:`read_excel` has deprecated the ``skip_footer`` parameter. Use ``skipfooter`` instead (:issue:`18836`)
 - The ``is_copy`` attribute is deprecated and will be removed in a future version (:issue:`18801`).
+- The ``convert_datetime64`` parameter in :func:`DataFrame.to_records` has been deprecated and the default value is now ``False``. The NumPy bug motivating this parameter has been resolved (:issue:`18160`).


say it will be removed

jreback · 2017-12-27T20:06:29Z

pandas/core/frame.py

@@ -1196,14 +1196,23 @@ def to_records(self, index=True, convert_datetime64=True):
        ----------
        index : boolean, default True
            Include index in resulting record array, stored in 'index' field
-        convert_datetime64 : boolean, default True
+        convert_datetime64 : boolean, optional
+            .. deprecated:: 0.23.0


right, but it still defaults to True (its just we are getting rid of it).

jreback · 2017-12-27T20:07:17Z

pandas/core/frame.py

+                          "deprecated and will be removed in a future "
+                          "version",
+                          FutureWarning, stacklevel=2)
+
        if index:


else: convert_datetime64 = True

jreback · 2017-12-27T20:07:44Z

pandas/tests/frame/test_convert_to.py

-
-        rs = df.to_records(convert_datetime64=False)
-        assert rs['index'][0] == df.index.values[0]
+        with tm.assert_produces_warning(FutureWarning):


add an example where we don't pass it, so it gets defaulted to True.

jreback · 2018-01-13T18:44:15Z

pandas/core/frame.py

            Whether to convert the index to datetime.datetime if it is a
            DatetimeIndex

        Returns
        -------
        y : recarray
        """
+
+        if convert_datetime64 is not None:


@jorisvandenbossche how should we handle this here. We really want to change the default to False in the future, but need True to make this backwards compat for now.

on 2nd thought, let's just change this to default to False. We have to change it now or later anyways.

I think we need to have to None as a default to detect the case that people now did convert_datetime64=False explicitly. Otherwise they don't see the deprecation warning, and their code will break once we remove the keyword.

jreback · 2018-02-24T17:26:37Z

can you rebase

jreback · 2018-03-02T11:38:50Z

pandas/core/frame.py

            Whether to convert the index to datetime.datetime if it is a
            DatetimeIndex

        Returns
        -------
        y : recarray
        """
+
+        if convert_datetime64 is not None:


on 2nd thought, let's just change this to default to False. We have to change it now or later anyways.

jreback · 2018-03-02T11:39:09Z

pandas/tests/frame/test_convert_to.py

+            result = df.to_records(convert_datetime64=True)['index'][0]
+            assert expected == result
+
+        expected = df.index[0]


move comment above expected

jreback · 2018-03-02T11:39:23Z

pandas/tests/frame/test_convert_to.py

+        assert expected == result
+
+        with tm.assert_produces_warning(FutureWarning):
+            rs = df.to_records(convert_datetime64=False)


can you test with True as well

jorisvandenbossche · 2018-03-26T19:41:22Z

(moving this to the main thread)

Which option is best depends on what the end goal here is.
Do we actually want to remove the option altogether in the future? In that case, we always need to warn if the keyword is manually specified, so we need a default of None to be able to catch this (which can then mean an actual default of True or False, which one of the two is independent from having None as the default)

Given the deprecation warning, it seems we actually want to remove it. So I think we need a default of None (and then best give a different warning in case True or False was specified).

But having a default of None, does not mean that if it is None, it should mean convert_datetime64=True as you indicate in your comment above. We can still actually change the behaviour at the same time if we want (the deprecation warning is currently for the keyword, not for the actual behaviour change I think?)

We could also delay the actual change in behaviour and first warn that it will change in the future (your option 1).
But given this is such a corner case and almost never the desired behaviour, I am not sure it is needed in this case.

jreback · 2018-03-30T20:50:01Z

pandas/core/frame.py

@@ -1187,7 +1187,7 @@ def from_records(cls, data, index=None, exclude=None, columns=None,

        return cls(mgr)

-    def to_records(self, index=True, convert_datetime64=True):
+    def to_records(self, index=True, convert_datetime64=False):


yes I think we need to make this None, then can detect that a user is passing this. Yes we are also completely changing the behavior of this, but we really want to remove this parameter.

jreback · 2018-03-30T20:52:02Z

pandas/core/frame.py

@@ -1187,7 +1187,7 @@ def from_records(cls, data, index=None, exclude=None, columns=None,

        return cls(mgr)

-    def to_records(self, index=True, convert_datetime64=True):
+    def to_records(self, index=True, convert_datetime64=False):


This is in-line with @jorisvandenbossche comments. We want to warn if a user is passing this at ALL, and we are changing the default. (so its now False if the parameter is not specified). can you reword as appropriate. ping when ready.

jreback · 2018-04-14T13:40:08Z

if you can update @reidy-p

reidy-p · 2018-04-14T21:42:28Z

@jreback I think this is ready but would be good if you could double-check the tests to make sure they make sense.

jreback · 2018-04-14T23:58:33Z

lgtm. thanks @reidy-p

jschendel reviewed Dec 21, 2017

View reviewed changes

gfyoung added Deprecate Functionality to remove in pandas Dtype Conversions Unexpected or buggy dtype conversions labels Dec 22, 2017

reidy-p force-pushed the to_records_datetimeindex branch from 8202f2c to 6a31ce3 Compare December 22, 2017 09:54

jreback requested changes Dec 23, 2017

View reviewed changes

jsexauer mentioned this pull request Dec 23, 2017

DEPR: Clean up list of deprecations from prior versions #6581

Closed

1 task

reidy-p force-pushed the to_records_datetimeindex branch 2 times, most recently from 5b4bc9c to cb0dfd5 Compare December 27, 2017 12:37

jreback requested changes Dec 27, 2017

View reviewed changes

reidy-p force-pushed the to_records_datetimeindex branch from c13b109 to ef6cf9b Compare December 27, 2017 21:30

reidy-p force-pushed the to_records_datetimeindex branch from ef6cf9b to 00b10db Compare January 12, 2018 21:13

jreback added this to the 0.23.0 milestone Jan 13, 2018

jreback reviewed Jan 13, 2018

View reviewed changes

reidy-p force-pushed the to_records_datetimeindex branch 3 times, most recently from d9c9d7c to eca4757 Compare March 1, 2018 22:00

jreback requested changes Mar 2, 2018

View reviewed changes

reidy-p force-pushed the to_records_datetimeindex branch from eca4757 to 94ee32b Compare March 3, 2018 12:42

reidy-p force-pushed the to_records_datetimeindex branch from 94ee32b to 32606c5 Compare March 17, 2018 23:12

reidy-p force-pushed the to_records_datetimeindex branch from 32606c5 to f4c6190 Compare March 25, 2018 16:12

reidy-p force-pushed the to_records_datetimeindex branch 4 times, most recently from eb0b207 to fd0f114 Compare April 1, 2018 13:09

jreback requested changes Apr 14, 2018

View reviewed changes

reidy-p added 5 commits April 14, 2018 15:05

DEPR: convert_datetime64 parameter in to_records()

65adc50

error in whatsnew

dfa867c

changing default to None

c13e724

convert_datetime64 defaults to True if not passed

f66dc69

change default back to False

9837eb0

reidy-p force-pushed the to_records_datetimeindex branch 2 times, most recently from b0cc2be to 42d75e0 Compare April 14, 2018 16:18

Make default for convert_datetime64=None and adjust tests

4a1b1ba

reidy-p force-pushed the to_records_datetimeindex branch from 42d75e0 to 4a1b1ba Compare April 14, 2018 17:19

jreback approved these changes Apr 14, 2018

View reviewed changes

jreback merged commit d5d5a71 into pandas-dev:master Apr 14, 2018

reidy-p deleted the to_records_datetimeindex branch April 15, 2018 11:14

H0R5E mentioned this pull request Aug 19, 2018

DOC: fix Dataframe.to_records examples #22419

Merged

4 tasks

jreback mentioned this pull request Nov 20, 2019

DEPR: deprecations log for removed issues #13777

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DEPR: convert_datetime64 parameter in to_records() #18902

DEPR: convert_datetime64 parameter in to_records() #18902

reidy-p commented Dec 21, 2017 •

edited

Loading

jschendel Dec 21, 2017

gfyoung Dec 22, 2017

gfyoung commented Dec 22, 2017

reidy-p commented Dec 22, 2017

codecov bot commented Dec 22, 2017 •

edited

Loading

jreback Dec 23, 2017

jreback Mar 25, 2018

reidy-p Mar 26, 2018

jreback Mar 30, 2018

jreback Mar 30, 2018

jreback Dec 23, 2017

jreback Dec 23, 2017

jreback Dec 23, 2017

jreback Dec 23, 2017

jreback Dec 27, 2017

jreback Dec 27, 2017

jreback Dec 27, 2017

jreback Jan 13, 2018

jreback Mar 2, 2018

jorisvandenbossche Mar 26, 2018

jreback commented Feb 24, 2018

jreback Mar 2, 2018

jreback Mar 2, 2018

jreback Mar 2, 2018

jorisvandenbossche commented Mar 26, 2018

jreback Mar 30, 2018

jreback Mar 30, 2018

jreback commented Apr 14, 2018

reidy-p commented Apr 14, 2018

jreback commented Apr 14, 2018

DEPR: convert_datetime64 parameter in to_records() #18902

DEPR: convert_datetime64 parameter in to_records() #18902

Conversation

reidy-p commented Dec 21, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gfyoung commented Dec 22, 2017

reidy-p commented Dec 22, 2017

codecov bot commented Dec 22, 2017 • edited Loading

Codecov Report

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

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

jreback commented Feb 24, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jorisvandenbossche commented Mar 26, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jreback commented Apr 14, 2018

reidy-p commented Apr 14, 2018

jreback commented Apr 14, 2018

reidy-p commented Dec 21, 2017 •

edited

Loading

codecov bot commented Dec 22, 2017 •

edited

Loading