Implement Series.datetime.floor #9488
Conversation
skirui-source
added
feature request
Codecov Report
@@ Coverage Diff @@
## branch-21.12 #9488 +/- ##
================================================
- Coverage 10.79% 10.66% -0.13%
================================================
Files 116 117 +1
Lines 18869 19721 +852
================================================
+ Hits 2036 2104 +68
- Misses 16833 17617 +784
Continue to review full report at Codecov.
|
|
rerun tests |
skirui-source
requested review from
galipremsagar,
mayankanand007 and
hyperbolic2346
| ) | ||
| ], | ||
| ) | ||
| @pytest.mark.parametrize("time_type", DATETIME_TYPES) |
There was a problem hiding this comment.
Docs say Timedelta is supported too, add those tests separately in test_timedelta.py?
There was a problem hiding this comment.
Sure, could you also take a look at the comment above here?
There was a problem hiding this comment.
I just pulled in and built docs for this PR and I can see it.
There was a problem hiding this comment.
@galipremsagar based on slack conversation with @shwina, I have updated the docstrings to state that it only returns Series for now. Adding floor and ceil methods to DatetimeIndex should be a separate PR, with no changes required on the libcudf side. However, some work on the C++ side will be required to support TimeDeltaIndex.
Sample error:
>>> import pandas
>>> import cudf
>>> data = ['10 day 5h 2 min 3us 10ns',
... '+22:39:19.999999',
... '2 day 4h 03:08:02.000045',
... '+21:15:45.999999']
>>> dtype = "timedelta64[ns]"
>>> psr = pandas.TimedeltaIndex(data, dtype=dtype)
>>> gsr = cudf.TimedeltaIndex(data, dtype=dtype)
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
File "/home/u00ubzohp7sU4seIFv357/dev/rapids/cudf/python/cudf/cudf/core/index.py", line 1931, in __init__
data = column.as_column(np.array(data, dtype=dtype))
ValueError: Could not convert object to NumPy timedeltaThanks to both of you for catching and clarifying this!
There was a problem hiding this comment.
Why not have support for datetime64 types(Both Series & Index) in this PR and have timedelta64 added as follow-up ? I don't understand why we choose to make a separate PR: #9554
There was a problem hiding this comment.
this came up as a recommendation from @shwina, so he might be the best person to answer
There was a problem hiding this comment.
I thought a separate PR would be appropriate as this PR didn't touch anything ceil related. Apologies for any confusion.;
There was a problem hiding this comment.
@mayankanand007 Can you create issues for ceil & floor support for timedelta64 types too?
galipremsagar
added
5 - Ready to Merge
|
@gpucibot merge |
This reverts commit 201f750.
Fixes: #7102 Replaces: [#9488](https://github.com/rapidsai/cudf/pull/9488/files) Authors: - Sheilah Kirui (https://github.com/skirui-source) - Mayank Anand (https://github.com/mayankanand007) Approvers: - GALI PREM SAGAR (https://github.com/galipremsagar) - Michael Wang (https://github.com/isVoid) - Bradley Dice (https://github.com/bdice) - Vyas Ramasubramani (https://github.com/vyasr) URL: #9571
Fixes: #7102