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

Consolidate binary ops into Frame #9357

Merged
merged 11 commits into from
Oct 13, 2021

Conversation

isVoid
Copy link
Contributor

@isVoid isVoid commented Oct 1, 2021

Partially solves #9038

This PR moves binary ops from Series and DataFrame into Frame to reduce duplications. Because the arguments takes a different order in series.py and dataframe.py resort to functools.wraps to reorder the arguments while keeping the docstrings the same.

@github-actions github-actions bot added the Python Affects Python cuDF API. label Oct 1, 2021
@codecov
Copy link

codecov bot commented Oct 2, 2021

Codecov Report

Merging #9357 (4b01515) into branch-21.12 (ab4bfaa) will decrease coverage by 0.01%.
The diff coverage is 0.00%.

Impacted file tree graph

@@               Coverage Diff                @@
##           branch-21.12    #9357      +/-   ##
================================================
- Coverage         10.79%   10.77%   -0.02%     
================================================
  Files               116      116              
  Lines             18869    19444     +575     
================================================
+ Hits               2036     2096      +60     
- Misses            16833    17348     +515     
Impacted Files Coverage Δ
python/cudf/cudf/core/_base_index.py 0.00% <0.00%> (ø)
python/cudf/cudf/core/dataframe.py 0.00% <0.00%> (ø)
python/cudf/cudf/core/frame.py 0.00% <0.00%> (ø)
python/cudf/cudf/core/index.py 0.00% <0.00%> (ø)
python/cudf/cudf/core/join/_join_helpers.py 0.00% <ø> (ø)
python/cudf/cudf/core/multiindex.py 0.00% <0.00%> (ø)
python/cudf/cudf/core/series.py 0.00% <0.00%> (ø)
python/cudf/cudf/core/udf/api.py 0.00% <0.00%> (ø)
python/cudf/cudf/core/udf/lowering.py 0.00% <0.00%> (ø)
python/cudf/cudf/core/udf/pipeline.py 0.00% <0.00%> (ø)
... and 66 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 e63f455...4b01515. Read the comment docs.

@isVoid isVoid marked this pull request as ready for review October 2, 2021 07:56
@isVoid isVoid requested a review from a team as a code owner October 2, 2021 07:56
@isVoid isVoid requested review from cwharris and shwina October 2, 2021 07:56
@vyasr
Copy link
Contributor

vyasr commented Oct 4, 2021

@isVoid The description probably needs an update, after we made those changes Friday night we're no longer making use of partialmethod here.

@isVoid isVoid added non-breaking Non-breaking change improvement Improvement / enhancement to an existing function labels Oct 4, 2021
@isVoid isVoid self-assigned this Oct 6, 2021
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.

One minor suggestion for improving the docstrings, otherwise LGTM.

python/cudf/cudf/core/frame.py Outdated Show resolved Hide resolved
isVoid and others added 2 commits October 6, 2021 15:57
Co-authored-by: Vyas Ramasubramani <vyas.ramasubramani@gmail.com>
python/cudf/cudf/core/frame.py Outdated Show resolved Hide resolved
python/cudf/cudf/core/frame.py Outdated Show resolved Hide resolved
python/cudf/cudf/core/frame.py Outdated Show resolved Hide resolved
python/cudf/cudf/core/frame.py Outdated Show resolved Hide resolved
python/cudf/cudf/core/frame.py Outdated Show resolved Hide resolved
python/cudf/cudf/core/frame.py Outdated Show resolved Hide resolved
python/cudf/cudf/core/frame.py Outdated Show resolved Hide resolved
python/cudf/cudf/core/frame.py Outdated Show resolved Hide resolved
python/cudf/cudf/core/frame.py Outdated Show resolved Hide resolved
python/cudf/cudf/core/frame.py Outdated Show resolved Hide resolved
Co-authored-by: GALI PREM SAGAR <sagarprem75@gmail.com>
@isVoid
Copy link
Contributor Author

isVoid commented Oct 12, 2021

rerun tests


# The above has wrapped binary op function from `Frame` with `wrapper`,
# copied module level attributes to wrapper (`__docs__` etc.) and set
# `__wrapped__` attribute of `wrapper` to `Frame` binop. Since this wrapper
Copy link
Contributor

Choose a reason for hiding this comment

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

This comment is out of date, now that we set __signature__ instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well actually it's not out of date. @functools.wraps still sets the __wrapped__ attribute of wrapper to wrapped_func, and this makes the signature mismatches with the actual parameter list of wrapper, which is why we need to set the __signature__ attr below. I updated the docs with some explanation of cpython behavior here to hopefully make it a bit clearer to understand.

@shwina
Copy link
Contributor

shwina commented Oct 13, 2021

@gpucibot merge

@rapids-bot rapids-bot bot merged commit bdd0922 into rapidsai:branch-21.12 Oct 13, 2021
rapids-bot bot pushed a commit that referenced this pull request Nov 2, 2021
This PR is a follow-up to #9357 that consolidates the comparison binops (eq, ne, lt, le, gt, ge) into Frame, enabling their use on DataFrame objects as well as Series. Note that pandas DataFrames do not support the fill_value parameter for these operations (unlike other binop functions), so we could choose to disable it for ours as well but I have not done so at present (I don't see a particularly strong reason to lean either way).

Authors:
  - Vyas Ramasubramani (https://github.com/vyasr)

Approvers:
  - https://github.com/brandon-b-miller

URL: #9542
rapids-bot bot pushed a commit that referenced this pull request Apr 6, 2022
…string (#10576)

This PR moves all the binary operation methods such as `Frame.add` into `IndexedFrame`. This removes these methods from `Index` objects to match pandas, where Index objects do not have these methods. I have also consolidated the docstrings for these methods using a single template and our `docutils`.

Note that this is technically a breaking change that I am implementing without a deprecation cycle; however, I feel comfortable doing so because these methods were introduced incidentally in #9357 and #9542 into 21.12, so just a couple of releases ago, and since they are not pandas APIs I doubt that they have made significant penetration into user workflows.

Authors:
  - Vyas Ramasubramani (https://github.com/vyasr)

Approvers:
  - GALI PREM SAGAR (https://github.com/galipremsagar)

URL: #10576
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
improvement Improvement / enhancement to an existing function non-breaking Non-breaking change Python Affects Python cuDF API.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants