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

[FEA] Add dot product binary op #8909

Merged
merged 20 commits into from
Sep 8, 2021

Conversation

charlesbluca
Copy link
Member

@charlesbluca charlesbluca commented Jul 30, 2021

Closes #2448

Implements dot product for dataframes and series by using cupy.ndarray.dot on their underlying values property.

@github-actions github-actions bot added the Python Affects Python cuDF API. label Jul 30, 2021
@charlesbluca charlesbluca added 2 - In Progress Currently a work in progress feature request New feature or request non-breaking Non-breaking change Python Affects Python cuDF API. and removed Python Affects Python cuDF API. labels Jul 30, 2021
@charlesbluca charlesbluca marked this pull request as ready for review August 12, 2021 23:03
@charlesbluca charlesbluca requested a review from a team as a code owner August 12, 2021 23:03
@charlesbluca charlesbluca added 3 - Ready for Review Ready for review by team and removed 2 - In Progress Currently a work in progress labels Aug 12, 2021
@charlesbluca charlesbluca changed the title [WIP] Add dot product binary op [FEA] Add dot product binary op Aug 12, 2021
python/cudf/cudf/tests/test_binops.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
@charlesbluca
Copy link
Member Author

rerun tests

@codecov
Copy link

codecov bot commented Aug 18, 2021

Codecov Report

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

Impacted file tree graph

@@               Coverage Diff               @@
##             branch-21.10    #8909   +/-   ##
===============================================
  Coverage                ?   10.77%           
===============================================
  Files                   ?      115           
  Lines                   ?    19138           
  Branches                ?        0           
===============================================
  Hits                    ?     2062           
  Misses                  ?    17076           
  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 e9caed3...f9a9200. Read the comment docs.

Copy link
Contributor

@marlenezw marlenezw left a comment

Choose a reason for hiding this comment

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

Changes look good to me!

@charlesbluca
Copy link
Member Author

rerun tests

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.

This looks fine overall. It's unfortunate that it has to bypass the other binop machinery, but since the dot product is not strictly column by column I don't see a good alternative. Also we're dispatching to cupy rather quickly so it's not a huge issue.

python/cudf/cudf/tests/test_binops.py Outdated Show resolved Hide resolved
python/cudf/cudf/tests/test_binops.py Outdated Show resolved Hide resolved
python/cudf/cudf/tests/test_array_function.py 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
@vyasr
Copy link
Contributor

vyasr commented Sep 7, 2021

@charlesbluca is this ready for another round of review?

@charlesbluca
Copy link
Member Author

Yup, added a special case in __array_function__ to return the proper output.

@vyasr vyasr self-requested a review September 7, 2021 16:47
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.

Looks good! I have a couple of higher-level questions, but nothing that needs to hold this up.

  1. I noticed that you added some more descriptive comments for reflected ops. Was that requested during review? Perhaps we should instead include links to the Python Data Model page, specifically the section that describes reflected binops.
  2. I get that we try to match pandas everywhere, including docs, but maintaining N copies of comments like the "Among flexible wrappers..." seems bound to get out of date. Do we really need that? If we do, perhaps we should use a formatted template for those docstrings to reduce repetition (although that would be out of scope for this PR).

@charlesbluca
Copy link
Member Author

Thanks @vyasr!

  1. Yeah, I did that based on a review thread with @skirui-source here; thanks for that link, it definitely explains the concept in greater detail and can be used in place of my explanation.
  2. Agreed, maintaining those docstrings is a lot of extra toil that can be avoided - are there docstrings in the codebase using templates currently?

@vyasr
Copy link
Contributor

vyasr commented Sep 7, 2021

Regarding templates: yes, but very haphazardly and certainly not with any consistent plan across the code base. I opened #9050 to discuss this further.

Feel free to merge this whenever you've updated the documentation on reflection as you see fit.

@charlesbluca
Copy link
Member Author

rerun tests

@charlesbluca
Copy link
Member Author

@gpucibot merge

@rapids-bot rapids-bot bot merged commit b10835c into rapidsai:branch-21.10 Sep 8, 2021
@charlesbluca charlesbluca deleted the add-dot-op branch July 19, 2022 14:26
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 feature request New feature or request non-breaking Non-breaking change Python Affects Python cuDF API.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[FEA] Dot Product
4 participants