Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Support round operation on datetime64[ns] datatypes #9820

Merged
merged 31 commits into from
Dec 10, 2021
Merged

Support round operation on datetime64[ns] datatypes #9820

merged 31 commits into from
Dec 10, 2021

Conversation

mayankanand007
Copy link
Contributor

@mayankanand007 mayankanand007 commented Dec 2, 2021 •
edited
Loading

This PR fixes #9652, by adding support for doing round operation on dtaetime64[ns] types, which is essentially supporting series.dt.round and DatetimeIndex.round.

In addition to this, we move the round implementation that is currently there from Frame to IndexedFrame as pd.Index doesn't support round. This is why we move this implementation to IndexedFrame, and add the code specifically for DatetimeIndex (for this PR), so as to avoid having a round method for another index types.

@mayankanand007 mayankanand007 self-assigned this Dec 2, 2021
@github-actions github-actions bot added Python Affects Python cuDF API. libcudf Affects libcudf (C++/CUDA) code. labels Dec 2, 2021
@mayankanand007 mayankanand007 added 2 - In Progress Currently a work in progress non-breaking Non-breaking change Cython feature request New feature or request labels Dec 2, 2021
@mayankanand007
Copy link
Contributor Author

rerun tests

@mayankanand007 mayankanand007 marked this pull request as ready for review December 3, 2021 20:08
@mayankanand007 mayankanand007 requested review from a team as code owners December 3, 2021 20:08
@mayankanand007 mayankanand007 added 3 - Ready for Review Ready for review by team and removed 2 - In Progress Currently a work in progress labels Dec 3, 2021
@mayankanand007 mayankanand007 requested review from shwina and vyasr and removed request for marlenezw December 3, 2021 20:09
bdice
bdice requested changes Dec 7, 2021
View reviewed changes
python/cudf/cudf/core/index.py Outdated Show resolved Hide resolved
python/cudf/cudf/core/index.py Outdated Show resolved Hide resolved
python/cudf/cudf/core/indexed_frame.py Outdated Show resolved Hide resolved
python/cudf/cudf/core/indexed_frame.py Outdated Show resolved Hide resolved
python/cudf/cudf/core/indexed_frame.py Outdated Show resolved Hide resolved
python/cudf/cudf/core/indexed_frame.py Outdated Show resolved Hide resolved
python/cudf/cudf/core/indexed_frame.py Outdated Show resolved Hide resolved
bdice
bdice approved these changes Dec 8, 2021
View reviewed changes
Copy link
Contributor

@bdice bdice left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The changeset looks good to me, thanks for iterating on this @mayankanand007! I think there are some opportunities to consolidate the public C++ API and Cython bindings. I will file an issue documenting my proposal when I get a chance. The summary is that I'd like to replace all the variations of round_day(column), ceil_hour(column), floor_microsecond(column) with round(column, freq), ceil(column, freq), or floor(column, freq) where freq is a public enum of supported rounding frequencies.

I'd like to see less boilerplate code repeated between ceil/floor/round in the tests (both Python and C++). I am approving because the new tests for round are aligned with the existing ceil/floor tests. A refactor could be done later.

@mayankanand007
Copy link
Contributor Author

rerun tests

vyasr
vyasr reviewed Dec 8, 2021
View reviewed changes
Copy link
Contributor

@vyasr vyasr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have a couple of small comments, but otherwise this looks good! I'll hold off on final approval pending @shwina's thoughts on the parameter name change.

cpp/tests/datetime/datetime_ops_test.cpp Show resolved Hide resolved
cpp/tests/datetime/datetime_ops_test.cpp Outdated Show resolved Hide resolved
python/cudf/cudf/core/index.py Show resolved Hide resolved
@vyasr vyasr self-requested a review December 9, 2021 17:27
vyasr
vyasr approved these changes Dec 9, 2021
View reviewed changes
Copy link
Contributor

@vyasr vyasr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. @mayankanand007 you'll need to fix the style checks, but I assume that the tests will pass after.

@mayankanand007
Copy link
Contributor Author

LGTM. @mayankanand007 you'll need to fix the style checks, but I assume that the tests will pass after.

Thanks @vyasr ! the style failure is due to a known issue (#9870), waiting for that to get merged. Does this PR need more approvals?

@bdice bdice added 5 - Ready to Merge Testing and reviews complete, ready to merge and removed 3 - Ready for Review Ready for review by team labels Dec 10, 2021
@mayankanand007
Copy link
Contributor Author

@gpucibot merge

@rapids-bot rapids-bot bot merged commit 45001f6 into rapidsai:branch-22.02 Dec 10, 2021
This was referenced Dec 10, 2021
Closed
Merged
rapids-bot bot pushed a commit that referenced this pull request Jan 4, 2022
@mayankanand007
Refactoring ceil/round/floor code for datetime64 types (#9926)
897a9ea 
This PR is a follow up to #9820 where @bdice and @vyasr raised the point of having a design such that we avoid writing bunch of boilerplate code, which is common in the implementations of ceil/round/floor. The aim is to reduce the total number of functions, as well as have a cleaner design.

Authors:
  - Mayank Anand (https://github.com/mayankanand007)

Approvers:
  - Ashwin Srinath (https://github.com/shwina)
  - Karthikeyan (https://github.com/karthikeyann)
  - Bradley Dice (https://github.com/bdice)

URL: #9926
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bdice bdice bdice approved these changes

@vyasr vyasr vyasr approved these changes

@skirui-source skirui-source Awaiting requested review from skirui-source skirui-source is a code owner automatically assigned from rapidsai/cudf-python-codeowners

@shwina shwina Awaiting requested review from shwina

@jrhemstad jrhemstad Awaiting requested review from jrhemstad

Assignees

@mayankanand007 mayankanand007

Labels
5 - Ready to Merge Testing and reviews complete, ready to merge feature request New feature or request libcudf Affects libcudf (C++/CUDA) code. non-breaking Non-breaking change Python Affects Python cuDF API.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[FEA] Support series.dt.round and DatetimeIndex.round operation
5 participants
@mayankanand007 @shwina @vyasr @bdice @jrhemstad