Skip to content

BUG: datetime64/timedelta64 comparisons return NotImplemented#23201

Merged
seberg merged 4 commits intonumpy:mainfrom
jbrockmendel:nat-defer
Feb 13, 2023
Merged

BUG: datetime64/timedelta64 comparisons return NotImplemented#23201
seberg merged 4 commits intonumpy:mainfrom
jbrockmendel:nat-defer

Conversation

@jbrockmendel
Copy link
Copy Markdown
Contributor

Closes #17017

Co-authored-by: Sebastian Berg <sebastian@sipsolutions.net>
@seberg seberg merged commit 80d5aeb into numpy:main Feb 13, 2023
@seberg
Copy link
Copy Markdown
Member

seberg commented Feb 13, 2023

Perfect, LGTM and thanks for the nice new test @jbrockmendel.

@jbrockmendel
Copy link
Copy Markdown
Contributor Author

booyah, thanks for the quick turnaround. im looking forward to un-xfailing the relevant pandas tests!

@jbrockmendel jbrockmendel deleted the nat-defer branch February 13, 2023 18:59
@charris charris added the 09 - Backport-Candidate PRs tagged should be backported label Feb 13, 2023
@charris
Copy link
Copy Markdown
Member

charris commented Feb 13, 2023

Should this be backported?

@seberg
Copy link
Copy Markdown
Member

seberg commented Feb 13, 2023

I would have leaned to not backport, since it is a very long-standing issue and maybe a bit subtle. But if it helps pandas, it should also be OK to do.

@jbrockmendel
Copy link
Copy Markdown
Contributor Author

im on a hotstreak, want to take a shot at the various .astpye(object) and .item() issues for dt64.

My best guess (figuring out where things are defined in numpy is hard) for a culprit is DATETIME_getitem in arraytypes.c.src, which calls convert_datetime_to_pyobject, which can return an int or datetime or date depending on values.

So I want to create a np.datetime64 object and return it from within convert_datetime_to_pyobject. My best guess here looks something like

NPY_NO_EXPORT PyObject *
convert_datetime_to_pyobject(npy_datetime dt, PyArray_DatetimeMetaData *meta)
{
    PyDatetimeScalarObject* dt64_obj;

    dt64_obj->obval = dt;
    dt64_obj->obmeta = meta;

    return (PyObject*)dt64_obj;
}

but im not seeing any way to actually initialize dt64_obj here. Is there a constructor somewhere?

@seberg
Copy link
Copy Markdown
Member

seberg commented Feb 13, 2023

but im not seeing any way to actually initialize dt64_obj here. Is there a constructor somewhere?

It is not a great function, but you can just use PyArray_Scalar() in getitem (the other function can be deleted). That seems to be the closest we got to a constructor.

The main issue is really all the fallout that might happen afterwards (like things magically working because NumPy converts the datetime to a python one).

@jbrockmendel
Copy link
Copy Markdown
Contributor Author

Thanks. I successfully got .item() and .astype(object) to work as expected! ... and got a segfault in test_array_coercion. But we're getting somewhere!

@jbrockmendel
Copy link
Copy Markdown
Contributor Author

minimal reproducer for the segfault:

td = np.timedelta64(123, "ns")
arr = np.array(0)
arr[()] = td  # <- segfault

troubleshooting...

charris pushed a commit to charris/numpy that referenced this pull request Feb 14, 2023
…23201)

* BUG: datetime64/timedelta64 comparisons return NotImplemented

* typo fixup

* Update numpy/core/src/multiarray/scalartypes.c.src

Co-authored-by: Sebastian Berg <sebastian@sipsolutions.net>

* Update numpy/core/src/multiarray/scalartypes.c.src

---------

Co-authored-by: Sebastian Berg <sebastian@sipsolutions.net>
@charris charris removed the 09 - Backport-Candidate PRs tagged should be backported label Feb 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

BUG: datetime64, timedelta64 comparisons do not return NotImplemented

3 participants