BUG: datetime64 hash. #14622
BUG: datetime64 hash. #14622
Conversation
walshb
force-pushed
the
bw-datetime-hash-3836
branch
from
b8a0ff6
to
f34afac
Compare
walshb
force-pushed
the
bw-datetime-hash-3836
branch
from
f34afac
to
db94d21
Compare
|
Can you add a test for this comment? |
|
Sorry for the conflict, should be straightforward to resolve |
numpy/core/src/multiarray/datetime.c
Outdated
| */ | ||
| NPY_NO_EXPORT npy_int64 | ||
| datetime_hash(PyArray_DatetimeMetaData *meta, npy_datetime dt) | ||
| { |
walshb
force-pushed
the
bw-datetime-hash-3836
branch
from
db94d21
to
5e88e5a
Compare
|
That requirement for hash(datetime.datetime) == hash(np.datetime64) meant lots of changes. |
|
Indeed it does. It might be worth discussing in that issue whether we care. I suppose we could merge this without resolving that, but I don't think we can consider this completely fixed until we address that too. |
|
Oh, I see you already made those changes - thanks! |
|
Some of the CI failures look real. @mattip, any idea what's up with PyPy? |
walshb
force-pushed
the
bw-datetime-hash-3836
branch
2 times, most recently
from
8f24882
to
16fdbcb
Compare
numpy/core/src/multiarray/datetime.c
Outdated
|
|
||
| if (meta->base == NPY_FR_GENERIC) { | ||
| /* XXX generic compares equal to *every* other base, so no single hash works. */ | ||
| return (td == -1) ? -2 : td; /* avoid -1 (error) */ |
There was a problem hiding this comment.
This is the path I am suggesting should raise.
There was a problem hiding this comment.
Ah. Yes. I agree, but that breaks another test, "test_correct_hash_dict" (in test_regression.py). So I left it as a XXX comment for now.
There was a problem hiding this comment.
I think that test is just not written well enough - it should be comparing hash(x) and x.__hash__(), which it had a close enough approximation to until this change
|
Re: CL failures, it seems to be some funny business with pypy. In pypy: np.timedelta64 == datetime.timedelta, but datetime.timedelta != np.timedelta64. Very strange. This doesn't affect np.datetime64 in pypy. |
|
PyPy uses the pure-python implementation of datetime, but it might have some issues. Let me take a look |
|
The timedelta._eq issue was fixed for CPython 3.7, but not backported. PyPy is backporting that now. |
walshb
force-pushed
the
bw-datetime-hash-3836
branch
from
16fdbcb
to
6317593
Compare
|
@mattip Cool. Thanks! |
walshb
force-pushed
the
bw-datetime-hash-3836
branch
from
6317593
to
355194b
Compare
walshb
force-pushed
the
bw-datetime-hash-3836
branch
from
355194b
to
9486e08
Compare
|
@walshb Are you still working on this? I think this is a good PR, just need some conflicting fix. |
walshb
force-pushed
the
bw-datetime-hash-3836
branch
from
9486e08
to
bf3b995
Compare
|
Travis problem, before the build started: "Unable to download 3.7 archive. The archive may not exist. Please consider a different version.". |
|
@Qiyu8 I rebased onto master, so there are no conflicts. But the build failed due to a missing Python interpreter for the s390x architecture on Travis. |
|
The CI problem you mentioned is a known issue (#16186) and not related to the changes in the PR, so nothing specifically needs to be done here. |
|
@walshb where does this PR stand? |
walshb
force-pushed
the
bw-datetime-hash-3836
branch
from
bf3b995
to
c5414ae
Compare
|
@mattip Seems this is ready for final review. |
Sorry for this late reply -- I've been trying to find some time to make one more change. |
#3836