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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Closed

pdnm wants to merge 53 commits into pandas-dev:master from pdnm:bugfix/string-timedelta2

Contributor

pdnm commented May 26, 2020 •

edited

Loading

Supersedes #23025

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

Victor added 30 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.


          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


          Updated more tests to avoid creating timedeltas from strings without …

e90a18b

…unit.


          Updated more tests to avoid creating timedeltas from strings without …

e0a55a8

…unit.


          Fixed failing test.

827aebf


          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


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

673ad19

…edelta2


          Merge remote-tracking branch 'origin/bugfix/string-timedelta2' into b…

eb7d3f8

…ugfix/string-timedelta2


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

bb40f77

…edelta2


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

2e331c1

…edelta2

# Conflicts:
#	doc/source/whatsnew/v0.24.0.txt
#	pandas/_libs/tslibs/timedeltas.pxd
#	pandas/_libs/tslibs/timedeltas.pyx
#	pandas/tests/indexes/timedeltas/test_ops.py


          Minor typo.

bada4a7


          Lint

8c0425a


          Using ValueError instead of Exception.

493a14b


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

e525ec4

…edelta2

# Conflicts:
#	pandas/_libs/tslibs/timedeltas.pyx


          Catching TypeError as well.

448b68a


          LINT

bd173a9

pdnm force-pushed the bugfix/string-timedelta2 branch from 7041bce to 5a0812c Compare

May 26, 2020 04:18

jbrockmendel reviewed

View reviewed changes

pandas/_libs/tslibs/timedeltas.pxd Outdated

@@ @@ -1,5 +1,7 @@ @@
               from numpy cimport int64_t
               # Exposed for tslib, not intended for outside use.
+              cpdef parse_timedelta_string(object ts, object specified_unit=*)
+              cdef int64_t cast_from_unit(object ts, object unit) except? -1

Member

jbrockmendel May 26, 2020

this has been moved to tslibs.conversion, so should be edited there rather than re-introduced here

Contributor Author

pdnm Jun 1, 2020

@jbrockmendel I fixed that and some other issues

pdnm changed the title ~~Bugfix/string timedelta2~~ [WIP] Bugfix/string timedelta2

pdnm force-pushed the bugfix/string-timedelta2 branch from c4c4ec6 to 507ff0f Compare

May 29, 2020 05:48

pep8speaks commented May 29, 2020 •

edited

Loading

Hello @pdnm! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2020-06-01 09:03:19 UTC


          Minor fix

b8031c2

pdnm force-pushed the bugfix/string-timedelta2 branch 7 times, most recently from 5cf2649 to bad005c Compare

June 1, 2020 08:38

pdnm changed the title ~~[WIP] Bugfix/string timedelta2~~ Bugfix/string timedelta2


          Merge branch 'master' into bugfix/string-timedelta2

cd17104

Fix linting issues, change single quotes to double quotes, add 'ns' to
timedelta string.

pdnm force-pushed the bugfix/string-timedelta2 branch from bad005c to cd17104 Compare

June 1, 2020 09:03

pdnm requested a review from jbrockmendel

June 1, 2020 10:12

jreback requested changes

View reviewed changes

Contributor

jreback left a comment

this needs to be handled at the entry points for users. we simply DO not allow a unit when the input is a string, full stop. We do this in to_datetime, please have a look; we would want to replicate this behavior.

doc/source/whatsnew/v1.1.0.rst

@@ @@ -701,6 +701,8 @@ Deprecations @@
                 instead (:issue:`34191`).
               - The ``squeeze`` keyword in the ``groupby`` function is deprecated and will be removed in a future version (:issue:`32380`)
+              - A timedelta passed a number string without a defined unit is deprecated (:issue:`12136`)

Contributor

jreback Jun 1, 2020

can you reference :class:`Timedelta` and pandas.to_timedelta here

Contributor

jreback Jun 1, 2020

why are we deprecating this? i would simply remove it as its wrong

Contributor Author

pdnm Jun 3, 2020

@jreback By "we simply DO not allow a unit when the input is a string"? Do you refer to the unit inside the string or the unit parameter? This change behaviors are like this:

Allows to_timedelta("1s"), used as example in https://pandas.pydata.org/docs/user_guide/timedeltas.html#to-timedelta
Allows to_timedelta("1", unit="s"), fixes bug BUG/API: to_timedelta unit-argument ignored for string input #12136
Disallows to_timedelta("1s", unit="s")
Warns to_timedelta("1")

I simply merged @Sup3rGeo's PR #23025 without changing any behaviors. Please let me know if you want something to be changed.

Contributor

jreback Jun 4, 2020 •

edited

Loading

we spent a long time on the PR but was ultimately not merged.because of the way it was structured. Please re-read those comments. We want to handle this at a much higher level, if you pass a unit then your input must be numeric, there is no other option. I don't think PR needs to be very complicated at all.

pandas/_libs/tslibs/timedeltas.pyx

                           have_value = 1
                           have_dot = 0
+                  # Consider units from outside

Contributor

jreback Jun 1, 2020

why are we doing the checking at this low level? this is simply a check on the user interface, e.g. if its a string, then no unit can be specified.

see how we do this in to_datetime

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)
+                          warnings.warn(

Contributor

jreback Jun 1, 2020

huh?

jreback added Error Reporting Timedelta labels

jreback changed the title ~~Bugfix/string timedelta2~~ BUG/API: to_timedelta unit-argument ignored for string input

jbrockmendel reviewed

View reviewed changes

pandas/_libs/tslibs/timedeltas.pyx

@@ @@ -174,6 +175,8 @@ cdef convert_to_timedelta64(object ts, object unit): @@
                   Return an ns based int64
                   """
+                  if unit is None:
+                      unit = 'ns'

Member

jbrockmendel Jun 1, 2020

where is this called from? could we type the argument as str and require the caller to pass it?

Member

jbrockmendel Jun 1, 2020

e.g. in array_to_timedelta64 below we should just do that check once at the top of the function, not in every step

jbrockmendel reviewed

View reviewed changes

pandas/_libs/tslibs/timedeltas.pyx

    
            @@ -260,7 +258,7 @@ def array_to_timedelta64(object[:] values, unit='ns', errors='raise'):
          
                  return iresult.base  # .base to access underlying np.ndarray

              cdef inline int64_t parse_timedelta_string(str ts) except? -1:

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

Member

jbrockmendel Jun 1, 2020

we really want to avoid weakening the type declarations

jbrockmendel reviewed

View reviewed changes

pandas/_libs/tslibs/timedeltas.pyx

+                  # Preserve unit if None, will be cast to nanoseconds
+                  # later on at the proper functions
+                  if unit is None or unit == 'M':

Member

jbrockmendel Jun 1, 2020

double quotes

jbrockmendel reviewed

View reviewed changes

pandas/core/computation/pytables.py

@@ @@ -200,7 +200,7 @@ def stringify(value): @@
                               v = v.tz_convert("UTC")
                           return TermValue(v, v.value, kind)
                       elif kind == "timedelta64" or kind == "timedelta":
-                          v = Timedelta(v, unit="s").value
+                          v = Timedelta(v).value

Member

jbrockmendel Jun 1, 2020

what type is "v" here? if it is an integer, this is a behavior change

jbrockmendel reviewed

View reviewed changes

pandas/core/tools/timedeltas.py

@@ @@ -108,7 +108,7 @@ def to_timedelta(arg, unit="ns", errors="raise"): @@
                   return _coerce_scalar_to_timedelta_type(arg, unit=unit, errors=errors)
-              def _coerce_scalar_to_timedelta_type(r, unit="ns", errors="raise"):
+              def _coerce_scalar_to_timedelta_type(r, unit=None, box=True, errors="raise"):

Member

jbrockmendel Jun 1, 2020

why adding an unused box kwarg?

jbrockmendel reviewed

View reviewed changes

pandas/tests/scalar/timedelta/test_timedelta.py

@@ @@ -535,7 +535,7 @@ def test_resolution_deprecated(self): @@
                       (Timedelta(0, unit="ns"), False),
                       (Timedelta(-10, unit="ns"), True),
                       (Timedelta(None), True),
-                      (NaT, True),
+                      (pd.NaT, True),

Member

jbrockmendel Jun 1, 2020

pls revert this

Contributor Author

pdnm commented Jun 8, 2020

Closing in favor of #34634

pdnm closed this

jreback mentioned this pull request

BUG/API: Disallow unit if input to Timedelta and to_timedelta is/contains str #34634

Merged

5 tasks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Error Reporting Timedelta