Accepts integer/float string with units and raises when unit is ambiguous (2) #23025

Sup3rGeo · 2018-10-07T16:26:53Z

Supersedes #21384.

closes BUG/API: to_timedelta unit-argument ignored for string input #12136
tests added / passed
passes git diff upstream/master -u -- "*.py" | flake8 --diff
whatsnew entry

…est.

… units.

pep8speaks · 2018-10-07T16:27:01Z

Hello @Sup3rGeo! Thanks for updating the PR.

Cheers ! There are no PEP8 issues in this Pull Request. 🍻

Comment last updated on January 08, 2019 at 15:23 Hours UTC

Sup3rGeo · 2018-10-07T16:30:36Z

@jorisvandenbossche Ok so I created another PR, we can continue on this one

Sup3rGeo · 2018-10-07T16:33:01Z

One point we have to decide: So far, Timedelta('3.1416') raised and Timedelta('2000') passed.

What do we want to do?

In this PR I am just raising for Timedelta('2000') as well and updated all tests that were not following this.

…unit.

doc/source/whatsnew/v0.24.0.txt

pandas/_libs/tslibs/timedeltas.pxd

pandas/core/tools/timedeltas.py

pandas/tests/scalar/timedelta/test_construction.py

Sup3rGeo · 2018-10-11T08:46:57Z

The failure in TravisCI seems to be something unrelated to this feature in question. Is there anything I should change related to that?

Sup3rGeo · 2018-10-23T08:48:23Z

@jreback I really don't know what to do to make this failing test in Travis CI pass. I need some help/pointers here.

jreback · 2018-11-11T18:37:33Z

pandas/_libs/tslibs/timedeltas.pyx

@@ -287,7 +287,7 @@ cdef inline _decode_if_necessary(object ts):
    return ts


-cdef inline parse_timedelta_string(object ts):
+cpdef inline parse_timedelta_string(object ts, specified_unit=None):


jreback · 2018-11-11T18:38:20Z

pandas/_libs/tslibs/timedeltas.pyx

-    if unit is None:
-        return 'ns'
-    elif unit == 'M':
+    if unit is None or unit == 'M':
        return unit


yes this is ok, can you add a comment

pandas/core/tools/timedeltas.py

jorisvandenbossche · 2018-11-21T15:41:58Z

doc/source/whatsnew/v0.24.0.txt

@@ -1132,9 +1132,9 @@ Timedelta
 - Fixed bug in adding a :class:`DataFrame` with all-`timedelta64[ns]` dtypes to a :class:`DataFrame` with all-integer dtypes returning incorrect results instead of raising ``TypeError`` (:issue:`22696`)
 - Bug in :class:`TimedeltaIndex` where adding a timezone-aware datetime scalar incorrectly returned a timezone-naive :class:`DatetimeIndex` (:issue:`23215`)
 - Bug in :class:`TimedeltaIndex` where adding ``np.timedelta64('NaT')`` incorrectly returned an all-`NaT` :class:`DatetimeIndex` instead of an all-`NaT` :class:`TimedeltaIndex` (:issue:`23215`)
+- Bug in :class:`Timedelta` where passing a string of a pure number would not take unit into account. Also raises for ambiguous/duplicate unit specification (:issue:`12136`)


Can you also mention to_timedelta, and also the change that unit now needs to be specified?

jorisvandenbossche · 2018-11-21T18:56:15Z

pandas/_libs/tslibs/timedeltas.pyx

    else:
        if have_value:
            raise ValueError("have leftover units")
        if len(number):
-            r = timedelta_from_spec(number, frac, 'ns')
-            result += timedelta_as_neg(r, neg)
+            raise ValueError("number string without units")


I think I mentioned it before, but I think we need to deprecate this first. We can here raise a deprecation warning when unit is None, and still fallback to unit='ns' for now

…edelta2

…edelta2 # Conflicts: # pandas/core/tools/timedeltas.py # pandas/tests/indexes/timedeltas/test_partial_slicing.py # pandas/tests/test_resample.py

…tion.

jreback · 2018-11-27T02:16:42Z

doc/source/whatsnew/v0.24.0.rst

@@ -1224,9 +1224,9 @@ Timedelta
 - Fixed bug in adding a :class:`DataFrame` with all-`timedelta64[ns]` dtypes to a :class:`DataFrame` with all-integer dtypes returning incorrect results instead of raising ``TypeError`` (:issue:`22696`)
 - Bug in :class:`TimedeltaIndex` where adding a timezone-aware datetime scalar incorrectly returned a timezone-naive :class:`DatetimeIndex` (:issue:`23215`)
 - Bug in :class:`TimedeltaIndex` where adding ``np.timedelta64('NaT')`` incorrectly returned an all-`NaT` :class:`DatetimeIndex` instead of an all-`NaT` :class:`TimedeltaIndex` (:issue:`23215`)
+- Bug in :class:`Timedelta` (and :func: `to_timedelta`) where passing a string of a pure number would not take unit into account. Also raises for ambiguous/duplicate unit specification and a number string without defined unit is deprecated (:issue:`12136`)


Bug in :class:`Timedelta` (and :func: `to_timedelta`) where passing a string of a pure number would not take the unit into account. Now raises for an ambiguous or duplicate unit specification.(:issue:`12136`)

put in the deprecation section
A timedelta passed a number string without a defined unit is deprecated (:issue:`12136`)

jreback · 2018-11-27T02:17:17Z

pandas/_libs/tslibs/timedeltas.pyx

@@ -165,6 +165,8 @@ cpdef convert_to_timedelta64(object ts, object unit):
    # kludgy here until we have a timedelta scalar


you can make unit=None in the signature, can you add a doc-string as well.

sorry, what should I add to the docstring exactly?

jreback · 2018-11-27T02:18:20Z

pandas/_libs/tslibs/timedeltas.pyx

+            warnings.warn(
+            "number string without units is deprecated and
+            " will raise an exception in future versions. Considering as nanoseconds.",
+            DeprecationWarning


use FutureWarning

jreback · 2018-11-27T02:18:51Z

pandas/_libs/tslibs/timedeltas.pyx

-        return 'ns'
-    elif unit == 'M':
+
+    # Preserve, will be dealt with eventually in other functions


what does this comment mean? can you reword to reflect your answer

ok, will try with a better comment.

jreback · 2018-11-27T02:19:39Z

pandas/core/arrays/timedeltas.py

@@ -521,6 +521,7 @@ def sequence_to_td64ns(data, copy=False, unit="ns", errors="raise"):
        # treat as multiples of the given unit.  If after converting to nanos,
        #  there are fractional components left, these are truncated
        #  (i.e. NOT rounded)
+        unit = unit if unit is not None else "ns"


if unit is None: unit = 'ns'

…edelta2

…edelta2 # Conflicts: # doc/source/whatsnew/v0.24.0.rst

jreback · 2018-12-03T01:35:15Z

can you merge master and i will look

…edelta2 # Conflicts: # doc/source/whatsnew/v0.24.0.rst # pandas/tests/util/test_hashing.py

jreback · 2018-12-23T23:11:36Z

can you merge master and update

jreback · 2019-01-05T15:39:34Z

can you merge master

…edelta2 # Conflicts: # doc/source/whatsnew/v0.24.0.rst # pandas/tests/frame/test_arithmetic.py

Sup3rGeo · 2019-01-08T17:08:30Z

Waiting for a seemingly bug in pytest

pytest-dev/pytest#4617

WillAyd · 2019-02-27T23:39:05Z

@Sup3rGeo can you merge master here? Looks like the relevant issue you linked is closed

jreback · 2019-03-20T02:02:11Z

can you merge master

jreback · 2019-04-05T00:54:52Z

would love to see this, can you merge master and update

WillAyd · 2019-05-03T05:38:27Z

Would be nice but I think this has gone stale. @Sup3rGeo please ping if you'd like to pick this one back up!

Victor added 12 commits October 7, 2018 16:06

Added null context manager as do_not_raise for parametrization in pyt…

ab5f7ba

…est.

Added test for pure integer strings and test for strings with/without…

7a38ecd

… units.

Added entry to whatsnew.

a65a35a

Changed Timedelta to raise for strings without unit.

57dea1d

WIP refactoring.

b8cf952

Deprecation warning when we don't have units in string numbers.

8154859

Fixed linting.

a90a992

Added to_timedelta test.

1184b6e

scalar case in to_timedelta checks for strings.

5e28b49

Updated test with more cases for to_timedelta.

fafe838

All tests passing. Implemented checks on parse_timedelta_string.

3ca0dae

Updated tests to avoid creating timedeltas from strings without unit.

8105f9c

Sup3rGeo mentioned this pull request Oct 7, 2018

Accepts integer/float string with units and raises when unit is ambiguous #21384

Closed

4 tasks

Victor added 2 commits October 7, 2018 19:13

Updated more tests to avoid creating timedeltas from strings without …

e90a18b

…unit.

Updated more tests to avoid creating timedeltas from strings without …

e0a55a8

…unit.

Sup3rGeo changed the title ~~Accepts integer/float string with units and raises when unit is ambiguous (2)~~ [WIP] Accepts integer/float string with units and raises when unit is ambiguous (2) Oct 7, 2018

Fixed failing test.

827aebf

jreback requested changes Oct 7, 2018

View reviewed changes

doc/source/whatsnew/v0.24.0.txt Outdated Show resolved Hide resolved

pandas/_libs/tslibs/timedeltas.pxd Outdated Show resolved Hide resolved

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

pandas/tests/scalar/timedelta/test_construction.py Outdated Show resolved Hide resolved

jreback added Timedelta Timedelta data type Error Reporting Incorrect or improved errors from pandas labels Oct 7, 2018

Victor added 5 commits October 8, 2018 18:05

Removed litle whitespace.

9bff504

Type specified unit as object.

b6c7d45

Removed newline.

9ff6ade

Moved default unit down to more core functions.

be93e4b

Fixed linting.

85f0103

Victor Maryama added 2 commits November 10, 2018 15:05

LINT: Fixed long lines.

64a4440

LINT: Fixed indentation of closing bracket.

4d26474

jreback requested changes Nov 11, 2018

View reviewed changes

jorisvandenbossche reviewed Nov 21, 2018

View reviewed changes

jorisvandenbossche mentioned this pull request Nov 21, 2018

BUG/API: to_timedelta unit-argument ignored for string input #12136

Closed

victor added 6 commits November 26, 2018 23:15

Merge remote-tracking branch 'upstream/master' into bugfix/string-tim…

db41054

…edelta2

Added comment.

2f53004

Merge remote-tracking branch 'upstream/master' into bugfix/string-tim…

fb40c94

…edelta2 # Conflicts: # pandas/core/tools/timedeltas.py # pandas/tests/indexes/timedeltas/test_partial_slicing.py # pandas/tests/test_resample.py

Casting unit to 'ns' if None.

f5e59ee

Updated documentation and issuing DeprecationWarning instead of excep…

e7266da

…tion.

Fixed linting.

b6a02df

jreback requested changes Nov 27, 2018

View reviewed changes

Victor Maryama added 4 commits November 27, 2018 18:55

DOC: Reorganized whatsnew

694abe9

CLN: Minor refactoring.

4d86ae1

Merge remote-tracking branch 'upstream/master' into bugfix/string-tim…

40b09f4

…edelta2

Merge remote-tracking branch 'upstream/master' into bugfix/string-tim…

a9c4675

…edelta2 # Conflicts: # doc/source/whatsnew/v0.24.0.rst

Merge remote-tracking branch 'upstream/master' into bugfix/string-tim…

95302de

…edelta2 # Conflicts: # doc/source/whatsnew/v0.24.0.rst # pandas/tests/util/test_hashing.py

Victor Maryama added 3 commits January 7, 2019 16:36

Merge remote-tracking branch 'upstream/master' into bugfix/string-tim…

661fd70

…edelta2 # Conflicts: # doc/source/whatsnew/v0.24.0.rst # pandas/tests/frame/test_arithmetic.py

Added default value to func declaration.

80a1161

Fixed bug in warning string.

dbe8fae

WillAyd closed this May 3, 2019

pdnm mentioned this pull request May 26, 2020

BUG/API: to_timedelta unit-argument ignored for string input #34379

Closed

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Accepts integer/float string with units and raises when unit is ambiguous (2) #23025

Accepts integer/float string with units and raises when unit is ambiguous (2) #23025

Sup3rGeo commented Oct 7, 2018

pep8speaks commented Oct 7, 2018 •

edited

Loading

Sup3rGeo commented Oct 7, 2018 •

edited

Loading

Sup3rGeo commented Oct 7, 2018

Sup3rGeo commented Oct 11, 2018

Sup3rGeo commented Oct 23, 2018

jreback Nov 11, 2018

jreback Nov 11, 2018

jorisvandenbossche Nov 21, 2018

jorisvandenbossche Nov 21, 2018

Sup3rGeo Nov 26, 2018

jreback Nov 27, 2018

jreback Nov 27, 2018

Sup3rGeo Nov 27, 2018

jreback Nov 27, 2018

Sup3rGeo Nov 27, 2018

jreback Nov 27, 2018

Sup3rGeo Nov 27, 2018

jreback Nov 27, 2018

Sup3rGeo Nov 27, 2018

jreback commented Dec 3, 2018

jreback commented Dec 23, 2018

jreback commented Jan 5, 2019

Sup3rGeo commented Jan 8, 2019

WillAyd commented Feb 27, 2019

jreback commented Mar 20, 2019

jreback commented Apr 5, 2019

WillAyd commented May 3, 2019

		@@ -165,6 +165,8 @@ cpdef convert_to_timedelta64(object ts, object unit):
		# kludgy here until we have a timedelta scalar

Accepts integer/float string with units and raises when unit is ambiguous (2) #23025

Accepts integer/float string with units and raises when unit is ambiguous (2) #23025

Conversation

Sup3rGeo commented Oct 7, 2018

pep8speaks commented Oct 7, 2018 • edited Loading

Comment last updated on January 08, 2019 at 15:23 Hours UTC

Sup3rGeo commented Oct 7, 2018 • edited Loading

Sup3rGeo commented Oct 7, 2018

Sup3rGeo commented Oct 11, 2018

Sup3rGeo commented Oct 23, 2018

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 Dec 3, 2018

jreback commented Dec 23, 2018

jreback commented Jan 5, 2019

Sup3rGeo commented Jan 8, 2019

WillAyd commented Feb 27, 2019

jreback commented Mar 20, 2019

jreback commented Apr 5, 2019

WillAyd commented May 3, 2019

pep8speaks commented Oct 7, 2018 •

edited

Loading

Sup3rGeo commented Oct 7, 2018 •

edited

Loading