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

Implement timestamp ceil #8942

Merged
merged 14 commits into from
Aug 25, 2021
Merged

Implement timestamp ceil #8942

merged 14 commits into from
Aug 25, 2021

Conversation

shaneding
Copy link
Contributor

@shaneding shaneding commented Aug 3, 2021 •
edited
Loading

Partly addresses #8682

This adds a ceil function for timestamp columns in libcudf. It is applied on fixed resolutions only.

@github-actions github-actions bot added the libcudf Affects libcudf (C++/CUDA) code. label Aug 3, 2021
@codecov
Copy link

codecov bot commented Aug 6, 2021 •
edited
Loading

Codecov Report

❗ No coverage uploaded for pull request base (branch-21.10@32f46f0). Click here to learn what that means.
The diff coverage is n/a.

❗ Current head b3ba1a1 differs from pull request most recent head 12e23d7. Consider uploading reports for the commit 12e23d7 to get more accurate results
Impacted file tree graph

@@               Coverage Diff               @@
##             branch-21.10    #8942   +/-   ##
===============================================
  Coverage                ?   10.76%           
===============================================
  Files                   ?      114           
  Lines                   ?    19088           
  Branches                ?        0           
===============================================
  Hits                    ?     2054           
  Misses                  ?    17034           
  Partials                ?        0

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 32f46f0...12e23d7. Read the comment docs.

@shaneding shaneding added feature request New feature or request non-breaking Non-breaking change labels Aug 11, 2021
@shaneding shaneding marked this pull request as ready for review August 11, 2021 17:59
@shaneding shaneding requested a review from a team as a code owner August 11, 2021 17:59
@shaneding shaneding requested review from trxcllnt and vuule August 11, 2021 17:59
@vuule
Copy link
Contributor

vuule commented Aug 11, 2021

is this still a work in progress?

@shaneding
Copy link
Contributor Author

@vuule The core PR is ready for review, just finalizing/refactoring the test cases.

@shaneding shaneding changed the title [WIP] Implement timestamp ceil Implement timestamp ceil Aug 12, 2021
@shaneding shaneding added the 3 - Ready for Review Ready for review by team label Aug 12, 2021
ttnghia
ttnghia requested changes Aug 12, 2021
View reviewed changes
cpp/include/cudf/datetime.hpp Outdated Show resolved Hide resolved
cpp/include/cudf/datetime.hpp Outdated Show resolved Hide resolved
cpp/include/cudf/datetime.hpp Outdated Show resolved Hide resolved
cpp/include/cudf/datetime.hpp Outdated Show resolved Hide resolved
cpp/include/cudf/datetime.hpp Outdated Show resolved Hide resolved
cpp/src/datetime/datetime_ops.cu Outdated Show resolved Hide resolved
cpp/src/datetime/datetime_ops.cu Outdated Show resolved Hide resolved
cpp/src/datetime/datetime_ops.cu Outdated Show resolved Hide resolved
cpp/src/datetime/datetime_ops.cu Outdated Show resolved Hide resolved
cpp/src/datetime/datetime_ops.cu Outdated Show resolved Hide resolved
vuule
vuule requested changes Aug 23, 2021
View reviewed changes
Copy link
Contributor

@vuule vuule left a comment

Choose a reason for hiding this comment

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

Looks like there's redundant dispatching here. We basically have three switches on the ceil type, feels like we should be able to do this only once.

cpp/src/datetime/datetime_ops.cu Show resolved Hide resolved
cpp/src/datetime/datetime_ops.cu Outdated Show resolved Hide resolved
@vuule
Copy link
Contributor

vuule commented Aug 25, 2021

@gpucibot merge

@rapids-bot rapids-bot bot merged commit 2a566dd into rapidsai:branch-21.10 Aug 25, 2021
@beckernick beckernick mentioned this pull request Aug 26, 2021
Closed
@shaneding shaneding mentioned this pull request Aug 28, 2021
Merged
rapids-bot bot pushed a commit that referenced this pull request Sep 4, 2021
@shaneding
Implemented bindings for ceil timestamp operation (#9141)
8d602b7 
Closes #8682. Added python bindings for `ceil` operation implemented in #8942.

Authors:
  - https://github.com/shaneding

Approvers:
  - Ashwin Srinath (https://github.com/shwina)
  - https://github.com/brandon-b-miller

URL: #9141
@mayankanand007 mayankanand007 mentioned this pull request Nov 10, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vuule vuule vuule approved these changes

@ttnghia ttnghia ttnghia approved these changes

@trxcllnt trxcllnt Awaiting requested review from trxcllnt

Assignees
No one assigned
Labels
3 - Ready for Review Ready for review by team feature request New feature or request libcudf Affects libcudf (C++/CUDA) code. non-breaking Non-breaking change
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@shaneding @vuule @ttnghia