[FEA] Add support for cudf.Timestamp
#16450
Conversation
Matt711
added
feature request
|
/ok to test |
|
/ok to test |
There was a problem hiding this comment.
I think the bulk of the changes are there. It still needs some tests.
cc. @mroeschke
| raise NotImplementedError("The method 'ceil' is not implemented.") | ||
|
|
||
| @classmethod | ||
| def combine(cls, date, time): |
There was a problem hiding this comment.
TODO: Type the arguments
| from cudf.core.scalar import Scalar | ||
|
|
||
|
|
||
| class Timestamp(Scalar): |
There was a problem hiding this comment.
I wonder if would be better to first get the implementations of cudf.Timestamp and cudf.Timedelta in and track their performance implications before replacing them throughout cudf (eg. here)
python/cudf/cudf/core/timestamp.py
Outdated
| super().__init__(ts) | ||
|
|
||
| @property | ||
| def value(self) -> np.datetime64: |
There was a problem hiding this comment.
Should the host value just be a pandas.Timestamp?
There was a problem hiding this comment.
So it looks like there's a cudf.Scalar.value which cudf.Timestamp inherits. It returns a np.datetime64 object. But there's also pd.Timestamp.value which returns an integer.
I think your right thatcudf.Timestamp.value should return pd.Timestamp(super().value).value. And I'll a separate property for access to the scalar value (np.datetime64).
python/cudf/cudf/core/timestamp.py
Outdated
| super().__init__(ts) | ||
|
|
||
| @property | ||
| def value(self) -> np.datetime64: |
There was a problem hiding this comment.
So it looks like there's a cudf.Scalar.value which cudf.Timestamp inherits. It returns a np.datetime64 object. But there's also pd.Timestamp.value which returns an integer.
I think your right thatcudf.Timestamp.value should return pd.Timestamp(super().value).value. And I'll a separate property for access to the scalar value (np.datetime64).
…nto feature/support-timestamp
…support-timestamp
|
/ok to test |
| **kwargs, | ||
| ): | ||
| pd_ts_kwargs = {k: v for k, v in kwargs.items() if k != "dtype"} | ||
| ts = pd.Timestamp(*args, **pd_ts_kwargs) |
There was a problem hiding this comment.
Is there a reason you are constructing a pandas Timestamp here? Is it being done only for validation purposes?
|
I'd like to have some more offline discussion about the implementations of |
Description
closes #12052 and closes #16420
Checklist