BUG: Fix nanosecond timedeltas #45108

deponovo · 2021-12-29T10:15:58Z

closes Summing nanoseconds time delta to timestamp #43764
tests added / passed
Ensure all linting tests pass, see here for how to run them
whatsnew entry

Fixes the construction of Timedelta objects with any nanosecond contribution.

…the timedelta constructor BUG: overriden the Timedelta.total_seconds() to return the correct value containing also the nanoseconds portion

jreback · 2021-12-29T12:26:40Z

pandas/_libs/tslibs/timedeltas.pyx

@@ -1268,7 +1268,16 @@ class Timedelta(_Timedelta):

            kwargs = {key: _to_py_int_float(kwargs[key]) for key in kwargs}

-            nano = convert_to_timedelta64(kwargs.pop('nanoseconds', 0), 'ns')
+            # GH43764, making sure any nanoseconds contributions from any kwarg is taken into consideration


umm this is just duplicating code from below

I don't know exactly which code you're referring to, but if I do not catch all nanoseconds contributions when a Timedelta instance is prepared using any of the "microseconds", "milliseconds" and "seconds" kwargs or any combination therefrom, the potential nanosecond contributions from every of those kwargs must be caught here, otherwise it will be lost information, e.g. Timedelta(seconds=1e-9, milliseconds=1e-5, microseconds=1e-1).

jreback · 2021-12-29T12:26:55Z

pandas/_libs/tslibs/timedeltas.pyx

@@ -1520,6 +1529,10 @@ class Timedelta(_Timedelta):
        div = other // self
        return div, other - div * self

+    # GH40946


doc string and types pls

Done on premise. (Depends on the decision to separate the issues in two PRs or not)

simonjayhawkins · 2021-12-29T17:33:12Z

pandas/_libs/tslibs/timedeltas.pyx

@@ -1520,6 +1529,10 @@ class Timedelta(_Timedelta):
        div = other // self
        return div, other - div * self

+    # GH40946
+    def total_seconds(self):
+        return self.value / 1e9


internally, ns precision is achieved by storing the value as a 64 bit integer.

once this is converted to a float, we cannot maintain ns precision. However, including the nanosecond component is probably more accurate than ignoring it.

I think we need more discussion on the issue before re-instating the nanosecond "precision".

from #31380 (comment)

... from "dubiously nanosecond-precision" to "microsecond-precision" ...

The change to ms precision was an intentional one but was not documented.

Changing it back would probably require a specific mention in the release note.

Also the tests are failing for the cases added when the precision of total_seconds changed, so this needs to be fixed also.

I also think that the 2 issue should be addressed independently.

Before I go and refactor/enhance the tests, it would be nice to have a categorical decision on this topic. Should the total_seconds part be a separate PR? I am not really sure why the nanosecond precision is a problem. The precise epoch in ns can be obtained by the value property.

Should the total_seconds part be a separate PR?

the two fixes are orthogonal? The override of the total_seconds method to fix #40946 and the change to __new__ to fix the constructor issue #43764 (comment)?

If so, I think should be two separate PRs.

yes pls split to a sepearate PR

simonjayhawkins · 2021-12-29T17:34:00Z

doc/source/whatsnew/v1.4.0.rst

@@ -236,6 +236,8 @@ Other enhancements
 - :meth:`is_list_like` now identifies duck-arrays as list-like unless ``.ndim == 0`` (:issue:`35131`)
 - :class:`ExtensionDtype` and :class:`ExtensionArray` are now (de)serialized when exporting a :class:`DataFrame` with :meth:`DataFrame.to_json` using ``orient='table'`` (:issue:`20612`, :issue:`44705`).
 - Add support for `Zstandard <http://facebook.github.io/zstd/>`_ compression to :meth:`DataFrame.to_pickle`/:meth:`read_pickle` and friends (:issue:`43925`)
+- :class:`Timedelta` now properly taking into account any nanoseconds contribution (:issue: `43764`)


Suggested change

- :class:`Timedelta` now properly taking into account any nanoseconds contribution (:issue: `43764`)

- :class:`Timedelta` now properly taking into account any nanoseconds contribution (:issue:`43764`)

simonjayhawkins · 2021-12-29T17:34:17Z

doc/source/whatsnew/v1.4.0.rst

@@ -236,6 +236,8 @@ Other enhancements
 - :meth:`is_list_like` now identifies duck-arrays as list-like unless ``.ndim == 0`` (:issue:`35131`)
 - :class:`ExtensionDtype` and :class:`ExtensionArray` are now (de)serialized when exporting a :class:`DataFrame` with :meth:`DataFrame.to_json` using ``orient='table'`` (:issue:`20612`, :issue:`44705`).
 - Add support for `Zstandard <http://facebook.github.io/zstd/>`_ compression to :meth:`DataFrame.to_pickle`/:meth:`read_pickle` and friends (:issue:`43925`)
+- :class:`Timedelta` now properly taking into account any nanoseconds contribution (:issue: `43764`)
+- :meth:`Timedelta.total_seconds()` now properly taking into account any nanoseconds contribution (:issue: `40946`)


Suggested change

- :meth:`Timedelta.total_seconds()` now properly taking into account any nanoseconds contribution (:issue: `40946`)

- :meth:`Timedelta.total_seconds()` now properly taking into account any nanoseconds contribution (:issue:`40946`)

jreback · 2021-12-29T17:54:47Z

@simonjayhawkins no more PRs on 1.4 actively trying to reduce the number. we can certainly decide later to backport or merge on the release.

simonjayhawkins · 2021-12-29T17:57:28Z

sure. we had the discussion on the last release about the milestone and adopted the blocker for rc label. Admittedly, I don't recall if that allowed adding new issues/prs to the milestone.

jreback · 2021-12-29T18:19:57Z

sure. we had the discussion on the last release about the milestone and adopted the blocker for rc label. Admittedly, I don't recall if that allowed adding new issues/prs to the milestone.

right but the point is i don't want to label these with a version until we are ready to merge (otherwise these tend to stick for no good reason)

jreback · 2021-12-30T15:43:22Z

pandas/_libs/tslibs/timedeltas.pyx

-            nano = convert_to_timedelta64(kwargs.pop('nanoseconds', 0), 'ns')
+            # GH43764, making sure any nanoseconds contributions from any kwarg
+            # is taken into consideration
+            nano = convert_to_timedelta64(


you are entirely duplicating L1283

My modifications removed "microseconds" till "seconds" from kwargs. Any other potential kwarg, such as year etc, are handled the same as the previous way, namely via L1283. Do you have other suggestions?

i actually don't have a problem with doing what you are doing but then you need to entirely remove L1282-1284 and handle all the kwargs

(well you still need the try/except).

this will also slow this function down a lot. rather than poping be explicit on the argument handling

Could you check the last changes? I profiled instantiation time and it is taking about 50% the time as in master.

def test(): pd.Timedelta(seconds=float(106751 * 24 * 3600), nanoseconds=1) pd.Timedelta(minutes=-7) pd.Timedelta(seconds=1234e-9) pd.Timedelta(seconds=1e-9, milliseconds=1e-5, microseconds=1e-1) pd.Timedelta(days=1, seconds=1e-9, milliseconds=1e-5, microseconds=1e-1)

pandas/tests/tslibs/test_timedeltas.py

simonjayhawkins · 2021-12-30T16:02:21Z

pandas/_libs/tslibs/timedeltas.pyx

+                    + kwargs.pop('microseconds', 0) * 1000
+                    + kwargs.pop('milliseconds', 0) * 1000000
+                    + kwargs.pop('seconds', 0) * 1000000000
+                ), 'ns'


because these are floats, I assume that this will lose precision near the limits. i.e. seconds = 106751 (days) * 24*3600 with a nanosecond component specified too?

Not really sure, but I would expect that when aiming at precision the inputs should instead be coded as integers by the caller? My motivation here is only to make the instantiation consistent in that, if float inputs are allowed, then any nanosecond contributions from any supplied kwargs must be taken into consideration

indeed, if any of these components is a float.

so pd.Timedelta(seconds=float(106751 * 24 * 3600), nanoseconds=1)

gives Timedelta('106751 days 00:00:00') whereas on master we get Timedelta('106751 days 00:00:00.000000001')

Could you check my last commit? It produces the same output as in master.

…nstructor

jreback

pls also add a whatsnew in 1.4. bug fixes in the datetime section

jreback · 2021-12-31T15:25:44Z

pandas/_libs/tslibs/timedeltas.pyx

                raise ValueError(
                    "cannot construct a Timedelta from the passed arguments, "
                    "allowed keywords are "
                    "[weeks, days, hours, minutes, seconds, "
                    "milliseconds, microseconds, nanoseconds]"
                )

+            # GH43764, making sure any nanoseconds contributions from any kwarg


this is not a useful comment as its not relevant for a current reader. However a comment explaining what you are doing would be useful.

jreback · 2021-12-31T15:28:17Z

pandas/_libs/tslibs/timedeltas.pyx

+                ) * 1_000_000_000
+            )
+
+            value = convert_to_timedelta64(


let's just directly create a np.timedelta64(ts, "ns") no reason to go thru the routine when we know exactly what we have.

Nice. Oversaw that. Performance is even better now.

DOC: moved the whatsnew entry to the timedelta section and added new comment

jreback · 2021-12-31T20:56:59Z

thanks @deponovo

jbrockmendel · 2022-01-05T18:50:53Z

pandas/_libs/tslibs/timedeltas.pyx

-                value = nano + convert_to_timedelta64(timedelta(**kwargs),
-                                                      'ns')
-            except TypeError as e:
+            if not cls._req_any_kwargs_new.intersection(kwargs):


This change means we now fail to raise on pd.Timedelta(days=2, foo=9)

I am going to create a PR for this..

deponovo added 4 commits December 29, 2021 10:47

BUG: now any nanoseconds contribution will be properly considered in …

cff6ee6

…the timedelta constructor BUG: overriden the Timedelta.total_seconds() to return the correct value containing also the nanoseconds portion

TST: extended tests to cover new timdelta construction possibilities

c0f8127

CLN: pre-commit cleanup

c9d7747

DOC: updated the whatsnew

e1110bf

deponovo changed the title ~~Fix nanosecond timedeltas~~ BUG: Fix nanosecond timedeltas Dec 29, 2021

jreback requested changes Dec 29, 2021

View reviewed changes

simonjayhawkins reviewed Dec 29, 2021

View reviewed changes

simonjayhawkins added Regression Functionality that used to work in a prior pandas version Timedelta Timedelta data type labels Dec 29, 2021

simonjayhawkins added this to the 1.4 milestone Dec 29, 2021

jreback removed this from the 1.4 milestone Dec 29, 2021

CLN: removed GH40946 related modification

4406deb

jreback reviewed Dec 30, 2021

View reviewed changes

pandas/tests/tslibs/test_timedeltas.py Show resolved Hide resolved

simonjayhawkins reviewed Dec 30, 2021

View reviewed changes

ENH: refactored logic for calculating nanoseconds in the timedelta co…

b8d37e7

…nstructor

jreback requested changes Dec 31, 2021

View reviewed changes

PERF: simplified construction of the timedelta

88145b5

DOC: moved the whatsnew entry to the timedelta section and added new comment

jreback added this to the 1.4 milestone Dec 31, 2021

jreback approved these changes Dec 31, 2021

View reviewed changes

jreback merged commit 8532faa into pandas-dev:master Dec 31, 2021

jbrockmendel reviewed Jan 5, 2022

View reviewed changes

deponovo mentioned this pull request Jan 6, 2022

BUG: raise on wrong keyword arguments in Timedelta #45227

Merged

4 tasks

jreback mentioned this pull request Jan 8, 2022

BUG: total_seconds() method returns zero for timedeltas smaller then 1 microsecond #40946

Closed

3 tasks

patrickmckenna mentioned this pull request May 17, 2022

BUG: Timedelta resolution is different depending on how the argument is passed #33992

Open

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

BUG: Fix nanosecond timedeltas #45108

BUG: Fix nanosecond timedeltas #45108

deponovo commented Dec 29, 2021 •

edited

Loading

jreback Dec 29, 2021

deponovo Dec 30, 2021

jreback Dec 29, 2021

deponovo Dec 30, 2021

simonjayhawkins Dec 29, 2021

deponovo Dec 30, 2021

simonjayhawkins Dec 30, 2021

jreback Dec 30, 2021

simonjayhawkins Dec 29, 2021

simonjayhawkins Dec 29, 2021

deponovo Dec 30, 2021

jreback commented Dec 29, 2021

simonjayhawkins commented Dec 29, 2021

jreback commented Dec 29, 2021

jreback Dec 30, 2021

deponovo Dec 30, 2021

jreback Dec 30, 2021

jreback Dec 30, 2021

deponovo Dec 31, 2021

simonjayhawkins Dec 30, 2021

deponovo Dec 30, 2021

simonjayhawkins Dec 30, 2021

deponovo Dec 31, 2021

jreback left a comment

jreback Dec 31, 2021

deponovo Dec 31, 2021

jreback Dec 31, 2021

deponovo Dec 31, 2021

jreback commented Dec 31, 2021

jbrockmendel Jan 5, 2022

deponovo Jan 6, 2022

	- :class:`Timedelta` now properly taking into account any nanoseconds contribution (:issue: `43764`)
	- :class:`Timedelta` now properly taking into account any nanoseconds contribution (:issue:`43764`)

	- :meth:`Timedelta.total_seconds()` now properly taking into account any nanoseconds contribution (:issue: `40946`)
	- :meth:`Timedelta.total_seconds()` now properly taking into account any nanoseconds contribution (:issue:`40946`)

BUG: Fix nanosecond timedeltas #45108

BUG: Fix nanosecond timedeltas #45108

Conversation

deponovo commented Dec 29, 2021 • edited Loading

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 29, 2021

simonjayhawkins commented Dec 29, 2021

jreback commented Dec 29, 2021

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 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 Dec 31, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

deponovo commented Dec 29, 2021 •

edited

Loading