[REVIEW] Implement cudf.DateOffset for months #6775

brandon-b-miller · 2020-11-16T16:25:22Z

Implements cudf.DateOffset - an object used for calendrical arithmetic, similar to pandas.DateOffset - for month units only.

Closes #6754

GPUtester · 2020-11-16T16:25:54Z

Please update the changelog in order to start CI tests.

View the gpuCI docs here.

codecov · 2020-11-16T19:28:09Z

Codecov Report

Merging #6775 (3dfbd68) into branch-0.18 (515a173) will increase coverage by 0.02%.
The diff coverage is 91.11%.

@@               Coverage Diff               @@
##           branch-0.18    #6775      +/-   ##
===============================================
+ Coverage        82.01%   82.04%   +0.02%     
===============================================
  Files               96       96              
  Lines            16340    16384      +44     
===============================================
+ Hits             13402    13443      +41     
- Misses            2938     2941       +3

Impacted Files	Coverage Δ
python/cudf/cudf/core/column/column.py	`88.03% <ø> (+0.10%)`	⬆️
python/cudf/cudf/core/tools/datetimes.py	`83.01% <88.23%> (+1.41%)`	⬆️
python/cudf/cudf/__init__.py	`100.00% <100.00%> (ø)`
python/cudf/cudf/core/column/datetime.py	`89.42% <100.00%> (+0.53%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 515a173...3dfbd68. Read the comment docs.

brandon-b-miller · 2020-12-01T20:11:04Z

This is ready for feedback

shwina · 2020-12-01T20:49:34Z

python/cudf/cudf/core/tools/datetimes.py

+        if op == 'sub':
+            months = -self._months
+        else:
+            months = self._months


Suggested change

if op == 'sub':

months = -self._months

else:

months = self._months

months = -self._months if op == "sub" else self._months

galipremsagar

Looks good to me. This is a good start, We can have support for the rest of the parameters too like minutes, seconds, microseconds, and nanoseconds in cudf.DateOffset by wrapping around a TimeDelta Scalar object internally. This can be tackled in a follow-up PR.

brandon-b-miller · 2020-12-02T16:04:19Z

Looks good to me. This is a good start, We can have support for the rest of the parameters too like minutes, seconds, microseconds, and nanoseconds in cudf.DateOffset by wrapping around a TimeDelta Scalar object internally. This can be tackled in a follow-up PR.

I had a similar thought. I am thinking this is a little more complicated than it might seem however. In Pandas, DateOffset respects calendrical arithmetic rather than absolute time arithmetic. Adding DateOffset(days=1) to a date might not be the same as adding 24 hours in some cases, for instance if we're crossing a daylight savings time boundary the result will be an hour off. So we'd need to create some logic in the python layer to adjust for this and create the right timedelta. From that perspective it might be better suited to put this logic in libcudf.

kkraus14 · 2020-12-04T18:34:46Z

python/cudf/cudf/core/tools/datetimes.py

+    def __init__(self, months):
+        self._months = months


We should follow the signature of Pandas DateOffset here: https://pandas.pydata.org/docs/reference/api/pandas.tseries.offsets.DateOffset.html

We should also validate that we support the parameters that are used.

python/cudf/cudf/core/tools/datetimes.py

kkraus14 · 2020-12-09T18:32:00Z

python/cudf/cudf/core/tools/datetimes.py

+    def _generate_column(self, size, op):
+        months = -self.months if op == "sub" else self.months
+        col = cudf.core.column.as_column(
+            months, dtype=np.dtype("int16"), length=size
+        )
+        return col


Does libcudf have an overload for the API that takes in a device scalar instead of a column? If not could you raise an issue?

raised #6990

python/cudf/cudf/core/tools/datetimes.py

Co-authored-by: Keith Kraus <[email protected]>

brandon-b-miller · 2020-12-11T19:42:54Z

@kkraus14 I think this is ready for another look.

python/cudf/cudf/core/tools/datetimes.py

kkraus14 · 2020-12-12T21:44:05Z

python/cudf/cudf/core/tools/datetimes.py

+            if k in all_possible_kwargs:
+                # Months must be int16
+                dtype = "int16" if k == "months" else None
+                kwds[k] = cudf.Scalar(v, dtype=dtype)


What does using cudf.Scalar objects do to the __repr__?

edit:
looking into this again, this approach actually won't work for a few reasons. This trips the validation inside pd.DateOffset for non-integer years but not months for example. I suppose we will really need to override __setattr__ which I was hoping to avoid :(

brandon-b-miller · 2020-12-15T00:08:10Z

Looking into these CI failures, but can't currently build the code. Stay tuned.

python/cudf/cudf/core/tools/datetimes.py

baseline implementation

ad09abf

brandon-b-miller added 2 - In Progress Currently a work in progress Python Affects Python cuDF API. Cython labels Nov 16, 2020

brandon-b-miller requested a review from a team as a code owner November 16, 2020 16:25

brandon-b-miller self-assigned this Nov 16, 2020

brandon-b-miller requested review from shwina and galipremsagar November 16, 2020 16:25

brandon-b-miller added 6 commits November 16, 2020 08:27

cleanup cython

ac47fdb

basic __repr__

3bce31e

basic testing

89b87bd

handle negative values

48d67b4

style

56bae18

changelog

5f45f34

brandon-b-miller added 4 commits November 30, 2020 07:25

Merge branch 'branch-0.17' into fea-month-sub-and-add

5005a57

fix negatives

cef0fc1

add a comment

69ebcf3

style

438866f

brandon-b-miller added 3 - Ready for Review Ready for review by team and removed 2 - In Progress Currently a work in progress labels Dec 1, 2020

brandon-b-miller changed the title ~~[WIP] Implement cudf.DateOffset for months~~ [REVIEW] Implement cudf.DateOffset for months Dec 1, 2020

shwina reviewed Dec 1, 2020

View reviewed changes

cleanup logic

e7145b9

galipremsagar approved these changes Dec 2, 2020

View reviewed changes

brandon-b-miller added feature request New feature or request non-breaking Non-breaking change labels Dec 2, 2020

kkraus14 changed the base branch from branch-0.17 to branch-0.18 December 4, 2020 18:21

Merge branch 'branch-0.18' into fea-month-sub-and-add

b8d824f

kkraus14 requested changes Dec 4, 2020

View reviewed changes

brandon-b-miller added 2 commits December 7, 2020 11:20

progress

93f5a48

tweak datetimes.py

16c0eab

kkraus14 requested changes Dec 9, 2020

View reviewed changes

brandon-b-miller added 2 commits December 9, 2020 11:33

inherit from pd.DateOffset

f079c54

intercept kwargs and convert to cudf scalars

23a553d

kkraus14 reviewed Dec 10, 2020

View reviewed changes

python/cudf/cudf/core/tools/datetimes.py Outdated Show resolved Hide resolved

brandon-b-miller and others added 4 commits December 10, 2020 12:04

pacify cython and libcudf

98b81ca

Update python/cudf/cudf/core/tools/datetimes.py

5f569e6

Co-authored-by: Keith Kraus <[email protected]>

Update python/cudf/cudf/core/tools/datetimes.py

8d5a0cc

Co-authored-by: Keith Kraus <[email protected]>

switch to NotImplementedError

a0c927b

kkraus14 requested changes Dec 12, 2020

View reviewed changes

brandon-b-miller added 2 commits December 14, 2020 05:10

address reviews

24bab96

updates

68af465

kkraus14 approved these changes Dec 15, 2020

View reviewed changes

python/cudf/cudf/core/tools/datetimes.py Show resolved Hide resolved

brandon-b-miller added 3 commits December 15, 2020 06:07

Merge branch 'branch-0.18' into fea-month-sub-and-add

a41f515

use a cudf python scalar instead of devicescalar in rolling

21957d1

comments and cleanup

3dfbd68

kkraus14 approved these changes Dec 15, 2020

View reviewed changes

kkraus14 added 5 - Ready to Merge Testing and reviews complete, ready to merge 6 - Okay to Auto-Merge and removed 3 - Ready for Review Ready for review by team labels Dec 15, 2020

rapids-bot bot merged commit 1963111 into rapidsai:branch-0.18 Dec 15, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[REVIEW] Implement cudf.DateOffset for months #6775

[REVIEW] Implement cudf.DateOffset for months #6775

brandon-b-miller commented Nov 16, 2020 •

edited

Loading

GPUtester commented Nov 16, 2020

codecov bot commented Nov 16, 2020 •

edited

Loading

brandon-b-miller commented Dec 1, 2020

shwina Dec 1, 2020

galipremsagar left a comment •

edited

Loading

brandon-b-miller commented Dec 2, 2020

kkraus14 Dec 4, 2020

kkraus14 Dec 9, 2020

brandon-b-miller Dec 11, 2020

brandon-b-miller commented Dec 11, 2020

kkraus14 Dec 12, 2020

brandon-b-miller Dec 14, 2020 •

edited

Loading

brandon-b-miller commented Dec 15, 2020

[REVIEW] Implement cudf.DateOffset for months #6775

[REVIEW] Implement cudf.DateOffset for months #6775

Conversation

brandon-b-miller commented Nov 16, 2020 • edited Loading

GPUtester commented Nov 16, 2020

codecov bot commented Nov 16, 2020 • edited Loading

Codecov Report

brandon-b-miller commented Dec 1, 2020

shwina Dec 1, 2020

Choose a reason for hiding this comment

galipremsagar left a comment • edited Loading

Choose a reason for hiding this comment

brandon-b-miller commented Dec 2, 2020

kkraus14 Dec 4, 2020

Choose a reason for hiding this comment

kkraus14 Dec 9, 2020

Choose a reason for hiding this comment

brandon-b-miller Dec 11, 2020

Choose a reason for hiding this comment

brandon-b-miller commented Dec 11, 2020

kkraus14 Dec 12, 2020

Choose a reason for hiding this comment

brandon-b-miller Dec 14, 2020 • edited Loading

Choose a reason for hiding this comment

brandon-b-miller commented Dec 15, 2020

brandon-b-miller commented Nov 16, 2020 •

edited

Loading

codecov bot commented Nov 16, 2020 •

edited

Loading

galipremsagar left a comment •

edited

Loading

brandon-b-miller Dec 14, 2020 •

edited

Loading