DEPR: Deprecate use of strings denoting units with 'M', 'Y' or 'y' in pd.to_timedelta (36666) #36838

avinashpancham · 2020-10-03T17:35:09Z

closes DEPR deprecate pd.to_timedelta('1 M') and pd.to_timedelta('1 Y') #36666
tests added / passed
passes black pandas
passes git diff upstream/master -u -- "*.py" | flake8 --diff
whatsnew entry

avinashpancham · 2020-10-03T17:37:37Z

Do we only want to deprecate variations with a space (pd.to_timedelta("1 Y")) or also without a space (pd.to_timedelta("1Y"))

pandas/core/tools/timedeltas.py

mroeschke · 2020-10-04T04:24:12Z

pandas/core/tools/timedeltas.py

+            raise ValueError(
+                "unit must not be specified if the input is/contains a str"
+            )
+        elif arg.upper().endswith(" M") or arg.upper().endswith(" Y"):


As mentioned in your other commented, we also want to catch arguments without the space as well e.g. "1Y".

Also we need a test for this warning

avinashpancham · 2020-10-04T13:49:45Z

Ah I see some tests are failing. I will update the code.

avinashpancham · 2020-10-04T17:13:16Z

All tests related to pd.to_timedelta are passing. The failed tests originate from another issue on master.

…line

… update tests

avinashpancham · 2020-10-12T06:25:30Z

Updated my code and merged master. Everything passes now.

mroeschke · 2020-10-12T06:39:28Z

pandas/tests/tools/test_to_timedelta.py

+        ],
+    )
+    def test_unambiguous_timedelta_values(self, val, warning):
+        with tm.assert_produces_warning(warning):


Could you but the github issue number here as a comment?

Done. CI fails due to other tests on master though. Will merge master later once that is solved.

jreback · 2020-10-12T15:12:36Z

pandas/core/tools/timedeltas.py

+        if unit is not None:
+            raise ValueError(
+                "unit must not be specified if the input is/contains a str"
+            )


hmm we already have this check on L103, so something must be going wrong here.

if you do: pd.to_timedelta("1 Y") then unit is not specified and by default None. L103 checks only if unit is "Y", "M", 'y' or 'm' (e.g. pd.to_timedelta(1, unit="Y") ), and therefore you never reach the conditional code.

jreback · 2020-10-12T15:14:52Z

pandas/tests/scalar/timedelta/test_timedelta.py

@@ -258,7 +258,9 @@ def test_unit_parser(self, units, np_unit, wrapper):
            if unit == "M":
                expected = Timedelta(np.timedelta64(2, "m").astype("timedelta64[ns]"))

-            result = to_timedelta(f"2{unit}")
+            warning = None if unit != "m" else FutureWarning


change this to parameterize this (e.g. elminate the for loop and parameterize on units)

then add a parameter whether should test for FutureWarning or not (could be bool is ok).

then in the body you can easily check this for both to_timedelta and Timedelta

We have approx 50 values that have no warning and just one ("m") that has a warning. IMO a parameter for this would be overkill. But if we want this then I can change it.

Updated the test. IMO test_unit_parser could be refactored even further: split in 2 tests: test_unit_parser_array and test_unit_parser_scalar and apply a parameter on the function that converts its to timedelta. Do we want to do this?

avinashpancham · 2020-10-15T20:57:33Z

I found out that the following was also possible: pd.to_timedelta(["1Y"]) or pd.to_timedelta(pd.Series(["1Y"])). So I updated to_timedelta to also give depr warnings in this situation. I'm not completely happy with this added code, so any feedback is appreciated

jreback

you need to do the warning inside the string to timedelta conversion code in timedelta.pyx itself;

jreback · 2020-10-16T01:39:10Z

pandas/core/tools/timedeltas.py

@@ -101,6 +110,16 @@ def to_timedelta(arg, unit=None, errors="raise"):
            "represent unambiguous timedelta values durations."
        )

+    if isinstance(arg, (list, tuple, ABCSeries, ABCIndexClass, np.ndarray)):


the warning shouldn't be here

jreback · 2020-10-16T01:39:32Z

pandas/_libs/tslibs/timedeltas.pyx

@@ -1467,6 +1468,16 @@ cdef _broadcast_floordiv_td64(
        return res


+def check_unambiguous_timedelta_values(object[:] values):


rather than this you can do it in the Timedelta string converter

jreback · 2020-10-16T01:39:47Z

pandas/core/tools/timedeltas.py

+                "unit must not be specified if the input is/contains a str"
+            )
+        elif re.search(r"^\d+\s?[M|Y|m|y]$", arg):
+            warnings.warn(


i wouldn't do this here at all

avinashpancham · 2020-10-18T13:35:33Z

Thanks @jreback , moved the warning to timedelta.pyx and the code is much cleaner now.

avinashpancham · 2020-10-27T08:03:45Z

@jreback implemented all suggestions, could you have a 2nd look? CI is still failing after merging master, but that is related to other issues than introduced in this PR.

avinashpancham · 2020-10-30T21:16:39Z

CI failed due to FATAL ERROR: Committing semi space failed. Allocation failed - process out of memory. Can someone please restart it? Thanks!

jreback · 2020-10-31T14:55:03Z

why is 'm' (lowercase) m deprecated? this is minutes as in not ambiguous. (agree on Y, y, M)

avinashpancham · 2020-10-31T14:58:05Z

My bad, I thought we wanted to deprecate all variations of m (lowercase and uppercase). Will revert it then.

jreback · 2020-10-31T15:17:36Z

My bad, I thought we wanted to deprecate all variations of m (lowercase and uppercase). Will revert it then.

kk great, otherwise lgtm. ping on green.

avinashpancham · 2020-10-31T18:37:42Z

@jreback reverted the depr of lowercase m, merged master and CI on green.

jreback · 2020-10-31T18:52:12Z

thanks @avinashpancham very nice!

… pd.to_timedelta (36666) (pandas-dev#36838)

avinashpancham commented Oct 3, 2020

View reviewed changes

pandas/core/tools/timedeltas.py Show resolved Hide resolved

avinashpancham changed the title ~~DEPR: Deprecate use of strings denoting units with 'M', 'Y', 'm' or 'y' in pd.to_timedelta~~ DEPR: Deprecate use of strings denoting units with 'M', 'Y', 'm' or 'y' in pd.to_timedelta (36666) Oct 3, 2020

mroeschke reviewed Oct 4, 2020

View reviewed changes

avinashpancham force-pushed the GH36666 branch from fde639e to c12eeaa Compare October 4, 2020 15:50

avinashpancham added 8 commits October 11, 2020 22:37

Throw warning when M,Y,m or y is used in an arg for pd.to_timedelta

de4fb4d

Update docstring by removing old logic and adding depr notice

61ec5b6

Add whatsnew entry

abb613e

Update wording in docs

4ccf43d

Place space at the end of string instead of at the beginning of next …

271f2fc

…line

Also catch month and year timedelta variations without a space

7b11a92

Add test for depr behavior

b39eadf

Catch deprecated patterns with a regex to prevent false positives and…

f56796a

… update tests

avinashpancham force-pushed the GH36666 branch from c12eeaa to f56796a Compare October 11, 2020 20:39

mroeschke reviewed Oct 12, 2020

View reviewed changes

Add description to unittest

ad38ad3

jreback requested changes Oct 12, 2020

View reviewed changes

jreback added Deprecate Functionality to remove in pandas Timedelta Timedelta data type labels Oct 12, 2020

avinashpancham added 2 commits October 15, 2020 22:24

Also find appearences of unambiguous timedelta values in containers

bc03065

Update test_unit_parser in test_timedelta.py

d491cce

jreback requested changes Oct 16, 2020

View reviewed changes

Move warnings code to timedeltas.pyx and update tests

76e6602

Update code for failing tests

cdc5821

avinashpancham force-pushed the GH36666 branch from cef33b7 to cdc5821 Compare October 18, 2020 15:26

avinashpancham mentioned this pull request Oct 24, 2020

DOC: freeze old whatsnew notes? #6856

Open

Replace ipython directives with code-blocks in whatsnew v0.15.0

8701f26

avinashpancham mentioned this pull request Oct 24, 2020

TST: Update unreliable num precision component of test_binary_arith_ops (GH37328) #37380

Merged

5 tasks

avinashpancham force-pushed the GH36666 branch from b83228b to c76486e Compare October 26, 2020 21:09

avinashpancham added 2 commits October 26, 2020 23:05

Merge remote-tracking branch 'upstream/master' into GH36666

783e039

Update newly added test

0cc6dff

avinashpancham force-pushed the GH36666 branch from c76486e to 0cc6dff Compare October 26, 2020 22:08

avinashpancham added 4 commits October 27, 2020 23:18

Merge remote-tracking branch 'upstream/master' into GH36666

6a02d05

Update test to prevent depr warning

71f801e

Merge remote-tracking branch 'upstream/master' into GH36666

e183f9b

Merge remote-tracking branch 'upstream/master' into GH36666

2d8c9fe

Merge remote-tracking branch 'upstream/master' into GH36666

1d2871b

avinashpancham added 2 commits October 31, 2020 16:23

Revert the depr of lowercase m

e01f75d

Revert more depr of m

5277943

avinashpancham changed the title ~~DEPR: Deprecate use of strings denoting units with 'M', 'Y', 'm' or 'y' in pd.to_timedelta (36666)~~ DEPR: Deprecate use of strings denoting units with 'M', 'Y' or 'y' in pd.to_timedelta (36666) Oct 31, 2020

Merge remote-tracking branch 'upstream/master' into GH36666

3b83aa0

jreback added this to the 1.2 milestone Oct 31, 2020

jreback mentioned this pull request Oct 31, 2020

DEPR: log of deprecations in 1.x (to be removed in 2.0) #30228

Closed

jreback approved these changes Oct 31, 2020

View reviewed changes

jreback merged commit d08dca6 into pandas-dev:master Oct 31, 2020

kesmit13 pushed a commit to kesmit13/pandas that referenced this pull request Nov 2, 2020

DEPR: Deprecate use of strings denoting units with 'M', 'Y' or 'y' in…

e4eea71

… pd.to_timedelta (36666) (pandas-dev#36838)

ukarroum pushed a commit to ukarroum/pandas that referenced this pull request Nov 2, 2020

DEPR: Deprecate use of strings denoting units with 'M', 'Y' or 'y' in…

ff611f6

… pd.to_timedelta (36666) (pandas-dev#36838)

MarcoGorelli mentioned this pull request Nov 7, 2022

DEPR: enforce a bunch #49552

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DEPR: Deprecate use of strings denoting units with 'M', 'Y' or 'y' in pd.to_timedelta (36666) #36838

DEPR: Deprecate use of strings denoting units with 'M', 'Y' or 'y' in pd.to_timedelta (36666) #36838

avinashpancham commented Oct 3, 2020

avinashpancham commented Oct 3, 2020

mroeschke Oct 4, 2020

avinashpancham Oct 4, 2020

avinashpancham commented Oct 4, 2020

avinashpancham commented Oct 4, 2020

avinashpancham commented Oct 12, 2020

mroeschke Oct 12, 2020

avinashpancham Oct 12, 2020 •

edited

Loading

jreback Oct 12, 2020

avinashpancham Oct 12, 2020 •

edited

Loading

jreback Oct 12, 2020

avinashpancham Oct 12, 2020

avinashpancham Oct 15, 2020 •

edited

Loading

avinashpancham commented Oct 15, 2020

jreback left a comment

jreback Oct 16, 2020

jreback Oct 16, 2020

jreback Oct 16, 2020

avinashpancham commented Oct 18, 2020

avinashpancham commented Oct 27, 2020

avinashpancham commented Oct 30, 2020

jreback commented Oct 31, 2020

avinashpancham commented Oct 31, 2020

jreback commented Oct 31, 2020

avinashpancham commented Oct 31, 2020

jreback commented Oct 31, 2020

		@@ -1467,6 +1468,16 @@ cdef _broadcast_floordiv_td64(
		return res


		def check_unambiguous_timedelta_values(object[:] values):

DEPR: Deprecate use of strings denoting units with 'M', 'Y' or 'y' in pd.to_timedelta (36666) #36838

DEPR: Deprecate use of strings denoting units with 'M', 'Y' or 'y' in pd.to_timedelta (36666) #36838

Conversation

avinashpancham commented Oct 3, 2020

avinashpancham commented Oct 3, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

avinashpancham commented Oct 4, 2020

avinashpancham commented Oct 4, 2020

avinashpancham commented Oct 12, 2020

Choose a reason for hiding this comment

avinashpancham Oct 12, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

avinashpancham Oct 12, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

avinashpancham Oct 15, 2020 • edited Loading

Choose a reason for hiding this comment

avinashpancham commented Oct 15, 2020

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

avinashpancham commented Oct 18, 2020

avinashpancham commented Oct 27, 2020

avinashpancham commented Oct 30, 2020

jreback commented Oct 31, 2020

avinashpancham commented Oct 31, 2020

jreback commented Oct 31, 2020

avinashpancham commented Oct 31, 2020

jreback commented Oct 31, 2020

avinashpancham Oct 12, 2020 •

edited

Loading

avinashpancham Oct 12, 2020 •

edited

Loading

avinashpancham Oct 15, 2020 •

edited

Loading