API: Timestamp and Timedelta .value changing in 2.0 #50891

MarcoGorelli · 2023-01-20T11:37:44Z

closes API: Timestamp and Timedelta .value changing in 2.0 #49076 (Replace xxxx with the GitHub issue number)
Tests added and passed if fixing a bug or adding a new feature
All code checks passed.
Added type annotations to new arguments/methods/functions.
Added an entry in the latest doc/source/whatsnew/vX.X.X.rst file if fixing a bug or adding a new feature.

MarcoGorelli · 2023-01-23T19:33:51Z

failed test:

FAILED pandas/tests/io/json/test_pandas.py::TestPandasContainer::test_date_index_and_values[date-True-epoch] - assert '{"1577836800...null,"a":"a"}' == '{"1577836800...null,"a":"a"}'
  - {"1577836800000":1577836800000,"null":null,"a":"a"}
  ?                                 ^^^^
  + {"1577836800000":1577836800000,"-6858695778871":null,"a":"a"}
  ?

can't get my head round it - cc @jbrockmendel @WillAyd in case you have any ideas, else I'll keep at it

renaming .value affects so much of the codebase..

jbrockmendel · 2023-01-23T19:35:46Z

can't get my head round it - cc @jbrockmendel @WillAyd in case you have any ideas, else I'll keep at it

best guess is in objToJSON.c there are a couple of get_long_attr(item, "value") calls that might not be caught by grepping for .value

WillAyd · 2023-01-23T21:04:17Z

I think the JSON module returns garbage values because of #49756

If you look, there are some branches in the code that do something like:

            if (PyObject_HasAttrString(item, "value")) {
                // see test_date_index_and_values for case with non-nano
                nanosecVal = get_long_attr(item, "value");
            } else {
                if (PyDelta_Check(item)) {
                    nanosecVal = total_seconds(item) *
                                 1000000000LL;  // nanoseconds per second
                } else {
                    // datetime.* objects don't follow above rules
                    nanosecVal = PyDateTimeToEpoch(item, NPY_FR_ns);
                }
            }

For the timedelta, this goes into total_seconds which is implemented as such:

static npy_float64 total_seconds(PyObject *td) {
    npy_float64 double_val;
    PyObject *value = PyObject_CallMethod(td, "total_seconds", NULL);
    double_val = PyFloat_AS_DOUBLE(value);
    Py_DECREF(value);
    return double_val;
}

I'm guessing that the object doesn't actually have total_seconds defined, and you end up getting pretty crazy results.

cc @lithomas1 who may be interested

WillAyd · 2023-01-23T21:07:17Z

As the master of code checks @MarcoGorelli if you had some kind of idea on how to set up CI so that all result = PyObject_... C functions are immediately followed by an if (result == NULL) { // handle error } that could help us clean up our extensions. Otherwise its probably worth doing a manual review of things as a pre-cursor to this PR

MarcoGorelli · 2023-01-24T13:42:22Z

Thanks, get_long_attr(item, "value") was indeed the part I needed to change!

As the master of code checks @MarcoGorelli if you had some kind of idea on how to set up CI so that all result = PyObject_... C functions are immediately followed by an if (result == NULL) { // handle error } that could help us clean up our extensions

😄 I'll take a look

MarcoGorelli

gonna block myself here, want to make sure this is really what's desired, as then the public method .value would become unavailable for old dates: #49076 (comment)

MarcoGorelli · 2023-02-02T09:50:21Z

gonna block myself here, want to make sure this is really what's desired, as then the public method .value would become unavailable for old dates: #49076 (comment)

have amended the error message to suggest using asm8 if nanoseconds aren't what they're after

pandas/_libs/tslibs/nattype.pyi

jbrockmendel · 2023-02-04T00:30:41Z

pandas/_libs/tslibs/timedeltas.pyi

@@ -89,6 +89,7 @@ class Timedelta(timedelta):
    max: ClassVar[Timedelta]
    resolution: ClassVar[Timedelta]
    value: int  # np.int64
+    _value: int  # np.int64


to be Technically Correct do we need to make value a property here?

pandas/core/arrays/_ranges.py

jbrockmendel · 2023-02-04T00:34:41Z

pandas/core/dtypes/cast.py

@@ -814,7 +814,7 @@ def infer_dtype_from_scalar(val, pandas_dtype: bool = False) -> tuple[DtypeObj,
            dtype = _dtype_obj
        else:
            dtype = np.dtype("m8[ns]")
-            val = np.timedelta64(val.value, "ns")
+            val = np.timedelta64(val._value, "ns")


i think this one is expecting the nanos value? (and probably needs to be updated to allow non-nano?)

jbrockmendel · 2023-02-04T00:36:36Z

pandas/core/indexes/interval.py

@@ -546,7 +546,7 @@ def _maybe_convert_i8(self, key):
            if lib.is_period(key):
                key_i8 = key.ordinal
            elif isinstance(key_i8, Timestamp):
-                key_i8 = key_i8.value
+                key_i8 = key_i8._value


looks like 1) this scalar path might not be necessary? and 2) there may be baked-in assumptions about everything being nano

good call - shall we make logic changes in a separate PR? I've opened #51196 about this nanosecond assumption here

separate PR seems fine

sure, thanks - any objections here? shall we move forward before merge conflicts arise?

mroeschke

Looks fairly good. Could this use a whatsnew note about .values raising a OverflowError for non nano unit?

MarcoGorelli · 2023-02-08T19:41:08Z

thanks - @jbrockmendel can confirm but I don't think this (non-nano timestamp) would've been available anyway in 1.5.x

jbrockmendel · 2023-02-08T21:47:32Z

I don't think this (non-nano timestamp) would've been available anyway in 1.5.x

correct

jbrockmendel

LGTM

jbrockmendel · 2023-02-09T01:04:05Z

thanks @MarcoGorelli

MarcoGorelli added 10 commits January 20, 2023 11:36

wip

84ea60f

wip

540113a

fixup test collection

82da04f

divide by zero to debug LOL

c026a4f

wip

378dbef

more fixups

dd6d8b4

fixup test unary ops

af97b06

ewm

aeb49d7

test offseetes

1f271ca

are we there yet?

3b96637

MarcoGorelli mentioned this pull request Jan 20, 2023

API: Timestamp and Timedelta .value changing in 2.0 #49076

Closed

Merge remote-tracking branch 'upstream/main' into timestamp-value

b0c4098

MarcoGorelli added 3 commits January 24, 2023 09:22

_value in ujson

a5aa8e0

Merge remote-tracking branch 'upstream/main' into timestamp-value

50476ca

🏷️ typing

a0cf7d4

MarcoGorelli marked this pull request as ready for review January 24, 2023 12:39

MarcoGorelli commented Jan 30, 2023

View reviewed changes

MarcoGorelli and others added 4 commits February 1, 2023 15:24

Merge remote-tracking branch 'upstream/main' into timestamp-value

255c4bd

wip

db72dd7

advise to use asm8 instead

16003f9

Merge branch 'main' into timestamp-value

0a2cec7

Merge remote-tracking branch 'upstream/main' into timestamp-value

2a11315

jbrockmendel reviewed Feb 2, 2023

View reviewed changes

pandas/_libs/tslibs/nattype.pyi Show resolved Hide resolved

fix test missing

2b4da6c

jbrockmendel reviewed Feb 4, 2023

View reviewed changes

pandas/core/arrays/_ranges.py Show resolved Hide resolved

jbrockmendel reviewed Feb 4, 2023

View reviewed changes

MarcoGorelli added 4 commits February 6, 2023 13:17

Merge remote-tracking branch 'upstream/main' into timestamp-value

8813511

make value property

9b50ffa

un-xfail test

79b3f13

Merge remote-tracking branch 'upstream/main' into timestamp-value

ea4d898

mroeschke reviewed Feb 8, 2023

View reviewed changes

mroeschke added this to the 2.0 milestone Feb 8, 2023

mroeschke added Timedelta Timedelta data type Timestamp pd.Timestamp and associated methods labels Feb 8, 2023

mroeschke approved these changes Feb 8, 2023

View reviewed changes

jbrockmendel approved these changes Feb 9, 2023

View reviewed changes

jbrockmendel merged commit b37321c into pandas-dev:main Feb 9, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

API: Timestamp and Timedelta .value changing in 2.0 #50891

API: Timestamp and Timedelta .value changing in 2.0 #50891

MarcoGorelli commented Jan 20, 2023 •

edited

Loading

MarcoGorelli commented Jan 23, 2023

jbrockmendel commented Jan 23, 2023

WillAyd commented Jan 23, 2023

WillAyd commented Jan 23, 2023 •

edited

Loading

MarcoGorelli commented Jan 24, 2023

MarcoGorelli left a comment

MarcoGorelli commented Feb 2, 2023

jbrockmendel Feb 4, 2023

jbrockmendel Feb 4, 2023

jbrockmendel Feb 4, 2023

MarcoGorelli Feb 6, 2023

jbrockmendel Feb 7, 2023

MarcoGorelli Feb 7, 2023

mroeschke left a comment

MarcoGorelli commented Feb 8, 2023

jbrockmendel commented Feb 8, 2023

jbrockmendel left a comment

jbrockmendel commented Feb 9, 2023

API: Timestamp and Timedelta .value changing in 2.0 #50891

API: Timestamp and Timedelta .value changing in 2.0 #50891

Conversation

MarcoGorelli commented Jan 20, 2023 • edited Loading

MarcoGorelli commented Jan 23, 2023

jbrockmendel commented Jan 23, 2023

WillAyd commented Jan 23, 2023

WillAyd commented Jan 23, 2023 • edited Loading

MarcoGorelli commented Jan 24, 2023

MarcoGorelli left a comment

Choose a reason for hiding this comment

MarcoGorelli commented Feb 2, 2023

jbrockmendel Feb 4, 2023

Choose a reason for hiding this comment

jbrockmendel Feb 4, 2023

Choose a reason for hiding this comment

jbrockmendel Feb 4, 2023

Choose a reason for hiding this comment

MarcoGorelli Feb 6, 2023

Choose a reason for hiding this comment

jbrockmendel Feb 7, 2023

Choose a reason for hiding this comment

MarcoGorelli Feb 7, 2023

Choose a reason for hiding this comment

mroeschke left a comment

Choose a reason for hiding this comment

MarcoGorelli commented Feb 8, 2023

jbrockmendel commented Feb 8, 2023

jbrockmendel left a comment

Choose a reason for hiding this comment

jbrockmendel commented Feb 9, 2023

MarcoGorelli commented Jan 20, 2023 •

edited

Loading

WillAyd commented Jan 23, 2023 •

edited

Loading