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.

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

ceil/floor for DatetimeIndex #9554

Merged

Conversation

mayankanand007
Copy link
Contributor

@mayankanand007 mayankanand007 commented Oct 28, 2021

Follow-up to #9571 where we add ceil and floor support for Series.

Here we add ceil and floor support to DatetimeIndex class. This PR is dependent on #9571 getting merged first since it assumes the libcudf implementation for floor exists.

@mayankanand007 mayankanand007 added the 2 - In Progress Currently a work in progress label Oct 28, 2021
@github-actions github-actions bot added the cuDF (Python) Affects Python cuDF API. label Oct 28, 2021
@mayankanand007 mayankanand007 added non-breaking Non-breaking change feature request New feature or request labels Oct 28, 2021
@mayankanand007 mayankanand007 self-assigned this Oct 28, 2021
@codecov
Copy link

codecov bot commented Oct 28, 2021

Codecov Report

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

Impacted file tree graph

@@               Coverage Diff               @@
##             branch-22.02    #9554   +/-   ##
===============================================
  Coverage                ?   10.60%           
===============================================
  Files                   ?      118           
  Lines                   ?    20090           
  Branches                ?        0           
===============================================
  Hits                    ?     2130           
  Misses                  ?    17960           
  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 3b38aa7...b8ff1b4. Read the comment docs.

@mayankanand007
Copy link
Contributor Author

mayankanand007 commented Oct 29, 2021

Based on a slack conversation with Ashwin:

The Index class in cuDF derives from a Frame class, which is a common base class for Series, DataFrame and Index.
Frame is where ceil is currently implemented. That ceil implementation does not currently support a freq= argument.
We should:

  1. Add a ceil (and floor) method in the subclasses DatetimeIndex and TimedeltaIndex that do support a freq argument.
  2. Deprecate the Frame.ceil and Frame.floor methods.

@mayankanand007 mayankanand007 added the 3 - Ready for Review Ready for review by team label Nov 2, 2021
@shwina
Copy link
Contributor

shwina commented Nov 2, 2021

but I wonder if we really need to deprecate the Frame methods.

My 2c is yes. I realize at this point that it's more effort to deprecate these APIs than just to continue supporting them. However, compatibility goes both ways: if users want to switch out Pandas with cuDF with minimal effort, doing the same the other way should also be minimal effort, and enabling APIs like these makes that more difficult.

Copy link
Contributor

@isVoid isVoid left a comment

Choose a reason for hiding this comment

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

I don't know the history but I imagined back in the days when UDFs where not handy, these functions are very useful rounding columns. Today with improvements in UDF it can be done like df.apply(lambda row: math.ceil(row['a']), axis=1). I see why we don't want to maintain two approach for the same goal.

python/cudf/cudf/tests/test_index.py Outdated Show resolved Hide resolved
python/cudf/cudf/core/index.py Outdated Show resolved Hide resolved
python/cudf/cudf/core/index.py Outdated Show resolved Hide resolved
@mayankanand007
Copy link
Contributor Author

@shwina should I then add Deprecation warning or no?

@shwina
Copy link
Contributor

shwina commented Nov 8, 2021

Yes please :)

@bdice bdice removed this from PR-WIP in v21.12 Release Nov 10, 2021
@bdice bdice added this to PR-WIP in v22.02 Release via automation Nov 10, 2021
@mayankanand007 mayankanand007 changed the base branch from branch-21.12 to branch-22.02 November 16, 2021 04:32
v22.02 Release automation moved this from PR-WIP to PR-Reviewer approved Nov 17, 2021
Copy link
Contributor

@isVoid isVoid left a comment

Choose a reason for hiding this comment

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

One small suggestion, else lgtm!

python/cudf/cudf/core/frame.py Outdated Show resolved Hide resolved
@shwina
Copy link
Contributor

shwina commented Nov 18, 2021

@gpucibot merge

@rapids-bot rapids-bot bot merged commit 406429a into rapidsai:branch-22.02 Nov 18, 2021
v22.02 Release automation moved this from PR-Reviewer approved to Done Nov 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3 - Ready for Review Ready for review by team cuDF (Python) Affects Python cuDF API. feature request New feature or request non-breaking Non-breaking change
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

4 participants