[REVIEW] Support `datetime64` in row-wise op #6415

isVoid · 2020-10-03T01:58:56Z

In solving this issue a bug from numpy surfaced: numpy/numpy#17428
find_common_types is not suggested. Should we start replacing the use of this function to np.promote_types?

jakirkham · 2020-10-03T03:35:29Z

Sounds like a good idea 🙂

seberg · 2020-10-03T19:55:57Z

Seems np.promote_types is currently limited to only two inputs, so np.result_type is probably more convenient.

EDIT: You also will have to do a try:/except Typerror: to fall back to object unfortunately.

kkraus14 · 2020-10-03T21:09:56Z

Thanks @seberg! We don't support object types in general so we should likely be fine 😄

kkraus14 · 2020-10-03T21:11:10Z

cc @brandon-b-miller for dtype related discussion since you've been working on implementing cudf dtypes.

brandon-b-miller · 2020-10-05T11:17:08Z

I'll look into if this affects anything and report back - I know we use it in a few places in join.

PR 6415

GPUtester · 2020-10-06T07:33:59Z

Please update the changelog in order to start CI tests.

View the gpuCI docs here.

isVoid

There are two other things I would like to have some comments:

Memory footprint. I could not come up with an easier solution when there are mixed resolution of datetime64, since the underlying values are stored w.r.t the resolution.
(Question thanks to @brandon-b-miller ) What are the expected behavior when overflow happens when converting to a higher resolution?

python/cudf/cudf/tests/test_dataframe.py

Skip one testcase due to pandas issue PR 6415

…6129

python/cudf/cudf/tests/test_dataframe.py

python/cudf/cudf/core/dataframe.py

- Separating pandas bug cases into separate assertion - Remove excess skipif PR 6415

…6129

Address review comments PR6415

python/cudf/cudf/core/dataframe.py

python/cudf/cudf/tests/test_dataframe.py

python/cudf/cudf/core/dataframe.py

codecov · 2020-10-08T01:24:15Z

Codecov Report

Merging #6415 into branch-0.17 will increase coverage by 0.38%.
The diff coverage is 92.30%.

@@               Coverage Diff               @@
##           branch-0.17    #6415      +/-   ##
===============================================
+ Coverage        82.72%   83.11%   +0.38%     
===============================================
  Files               90       90              
  Lines            14444    14816     +372     
===============================================
+ Hits             11949    12314     +365     
- Misses            2495     2502       +7

Impacted Files	Coverage Δ
python/cudf/cudf/utils/dtypes.py	`88.54% <84.61%> (+1.63%)`	⬆️
python/cudf/cudf/core/dataframe.py	`90.75% <100.00%> (+0.16%)`	⬆️
python/cudf/cudf/io/feather.py	`100.00% <0.00%> (ø)`
python/cudf/cudf/comm/serialize.py	`0.00% <0.00%> (ø)`
python/cudf/cudf/_fuzz_testing/io.py	`0.00% <0.00%> (ø)`
python/custreamz/custreamz/_version.py	`0.00% <0.00%> (ø)`
python/dask_cudf/dask_cudf/_version.py	`0.00% <0.00%> (ø)`
python/dask_cudf/dask_cudf/io/tests/test_csv.py	`100.00% <0.00%> (ø)`
python/dask_cudf/dask_cudf/io/tests/test_orc.py	`100.00% <0.00%> (ø)`
python/dask_cudf/dask_cudf/io/tests/test_json.py	`100.00% <0.00%> (ø)`
... and 38 more

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 b50662d...573640c. Read the comment docs.

- Handle both type and value exception for result type - Comment to explain why test memory idendity - Use with pytest.raise block

python/cudf/cudf/core/dataframe.py

Addresses review comments: - Using a wrapper of np.find_common_type to accomodate future need of cudf type promotion rules - Avoids the need to modify dataframe by deep copying underlying values used for row-wise op. PR 6415

…6129

python/cudf/cudf/core/dataframe.py

CHANGELOG.md

python/cudf/cudf/core/dataframe.py

…6129

python/cudf/cudf/core/dataframe.py

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

python/cudf/cudf/core/dataframe.py

kkraus14 · 2020-10-12T17:18:48Z

cc @brandon-b-miller to review again before we merge

brandon-b-miller

LGTM!

initial

a6a8c86

isVoid added 2 - In Progress Currently a work in progress Python Affects Python cuDF API. labels Oct 3, 2020

isVoid requested review from shwina and beckernick October 3, 2020 01:58

isVoid requested a review from a team as a code owner October 3, 2020 01:58

isVoid requested a review from kkraus14 October 3, 2020 01:58

isVoid marked this pull request as draft October 3, 2020 02:07

Initial implementations + test, with a few nuances to fix

c6f7990

PR 6415

isVoid commented Oct 6, 2020

View reviewed changes

python/cudf/cudf/tests/test_dataframe.py Outdated Show resolved Hide resolved

python/cudf/cudf/tests/test_dataframe.py Show resolved Hide resolved

Michael Wang added 3 commits October 6, 2020 20:30

Force convert to float64 for nullable datetime column

4b78171

Skip one testcase due to pandas issue PR 6415

Merge branch 'branch-0.17' of https://github.com/rapidsai/cudf into i…

902b13a

…6129

changelog

305f1dc

isVoid marked this pull request as ready for review October 7, 2020 03:38

isVoid changed the title ~~[WIP] Support datetime64[ms] in row-wise op~~ [REVIEW] Support datetime64[ms] in row-wise op Oct 7, 2020

isVoid added 3 - Ready for Review Ready for review by team and removed 2 - In Progress Currently a work in progress labels Oct 7, 2020

kkraus14 requested changes Oct 7, 2020

View reviewed changes

python/cudf/cudf/tests/test_dataframe.py Outdated Show resolved Hide resolved

python/cudf/cudf/tests/test_dataframe.py Outdated Show resolved Hide resolved

python/cudf/cudf/core/dataframe.py Outdated Show resolved Hide resolved

Michael Wang added 3 commits October 7, 2020 12:45

Address review comments

1c2a5ae

- Separating pandas bug cases into separate assertion - Remove excess skipif PR 6415

Merge branch 'branch-0.17' of https://github.com/rapidsai/cudf into i…

c24bce6

…6129

Replace use of find_common_type with result_type

83e6bf6

Address review comments PR6415

kkraus14 requested changes Oct 7, 2020

View reviewed changes

python/cudf/cudf/core/dataframe.py Outdated Show resolved Hide resolved

python/cudf/cudf/tests/test_dataframe.py Outdated Show resolved Hide resolved

python/cudf/cudf/tests/test_dataframe.py Outdated Show resolved Hide resolved

seberg reviewed Oct 7, 2020

View reviewed changes

python/cudf/cudf/core/dataframe.py Outdated Show resolved Hide resolved

Address review comments

2df7ede

- Handle both type and value exception for result type - Comment to explain why test memory idendity - Use with pytest.raise block

kkraus14 reviewed Oct 8, 2020

View reviewed changes

python/cudf/cudf/core/dataframe.py Outdated Show resolved Hide resolved

Michael Wang added 3 commits October 8, 2020 16:22

Wrapper find_common_type; avoid df mod

6175703

Addresses review comments: - Using a wrapper of np.find_common_type to accomodate future need of cudf type promotion rules - Avoids the need to modify dataframe by deep copying underlying values used for row-wise op. PR 6415

Merge branch 'branch-0.17' of https://github.com/rapidsai/cudf into i…

a923bcf

…6129

Style

920ad21

brandon-b-miller reviewed Oct 9, 2020

View reviewed changes

python/cudf/cudf/core/dataframe.py Show resolved Hide resolved

brandon-b-miller reviewed Oct 9, 2020

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

kkraus14 reviewed Oct 9, 2020

View reviewed changes

python/cudf/cudf/core/dataframe.py Outdated Show resolved Hide resolved

Michael Wang added 4 commits October 9, 2020 11:12

Zero-copy astype

abe09d1

changelog

6fc8877

Merge branch 'branch-0.17' of https://github.com/rapidsai/cudf into i…

2b48312

…6129

Removed memory test; added more test cases

e721e7c

kkraus14 reviewed Oct 9, 2020

View reviewed changes

python/cudf/cudf/core/dataframe.py Outdated Show resolved Hide resolved

Explicit copy=False for astype

f49d945

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

isVoid commented Oct 9, 2020

View reviewed changes

python/cudf/cudf/core/dataframe.py Outdated Show resolved Hide resolved

Also explicit copy=False for astype

6e524ed

kkraus14 approved these changes Oct 9, 2020

View reviewed changes

kkraus14 changed the title ~~[REVIEW] Support datetime64[ms] in row-wise op~~ [REVIEW] Support datetime64 in row-wise op Oct 9, 2020

Merge branch 'branch-0.17' into i6129

573640c

brandon-b-miller approved these changes Oct 12, 2020

View reviewed changes

kkraus14 added 5 - Ready to Merge Testing and reviews complete, ready to merge and removed 3 - Ready for Review Ready for review by team labels Oct 12, 2020

isVoid mentioned this pull request Oct 12, 2020

[REVIEW] Create agg() function for dataframes #6483

Merged

kkraus14 merged commit 8c32b80 into rapidsai:branch-0.17 Oct 12, 2020

beckernick mentioned this pull request Jun 30, 2021

[FEA] Row-wise operations support for DateTime columns. #6129

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[REVIEW] Support `datetime64` in row-wise op #6415

[REVIEW] Support `datetime64` in row-wise op #6415

isVoid commented Oct 3, 2020 •

edited

Loading

jakirkham commented Oct 3, 2020

seberg commented Oct 3, 2020 •

edited

Loading

kkraus14 commented Oct 3, 2020

kkraus14 commented Oct 3, 2020

brandon-b-miller commented Oct 5, 2020

GPUtester commented Oct 6, 2020

isVoid left a comment

codecov bot commented Oct 8, 2020 •

edited

Loading

kkraus14 commented Oct 12, 2020

brandon-b-miller left a comment

[REVIEW] Support datetime64 in row-wise op #6415

[REVIEW] Support datetime64 in row-wise op #6415

Conversation

isVoid commented Oct 3, 2020 • edited Loading

jakirkham commented Oct 3, 2020

seberg commented Oct 3, 2020 • edited Loading

kkraus14 commented Oct 3, 2020

kkraus14 commented Oct 3, 2020

brandon-b-miller commented Oct 5, 2020

GPUtester commented Oct 6, 2020

isVoid left a comment

Choose a reason for hiding this comment

codecov bot commented Oct 8, 2020 • edited Loading

Codecov Report

kkraus14 commented Oct 12, 2020

brandon-b-miller left a comment

Choose a reason for hiding this comment

[REVIEW] Support `datetime64` in row-wise op #6415

[REVIEW] Support `datetime64` in row-wise op #6415

isVoid commented Oct 3, 2020 •

edited

Loading

seberg commented Oct 3, 2020 •

edited

Loading

codecov bot commented Oct 8, 2020 •

edited

Loading