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

[REVIEW] Fix concatenation of cudf.RangeIndex #8970

Merged
merged 5 commits into from
Aug 5, 2021

Conversation

galipremsagar
Copy link
Contributor

Fixes: #6872

In cudf, we have been concatenating a collection of RangeIndex's by materializing each one of them, but instead we should rather be introspecting each RangeIndex to decide whether to materialize of not. This PR fixes it.

@galipremsagar galipremsagar added bug Something isn't working 3 - Ready for Review Ready for review by team Python Affects Python cuDF API. 4 - Needs cuDF (Python) Reviewer non-breaking Non-breaking change labels Aug 5, 2021
@galipremsagar galipremsagar self-assigned this Aug 5, 2021
@galipremsagar galipremsagar requested review from a team as code owners August 5, 2021 17:34
@quasiben
Copy link
Member

quasiben commented Aug 5, 2021

LGTM. cc @charlesbluca who was also looking at inefficiencies in the pack unpack work

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.

Thanks for unblocking CI! I intended to just leave a couple of optional code quality improvements, but I think there's an issue with the test, can you double-check?

python/cudf/cudf/core/index.py 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
python/cudf/cudf/tests/test_index.py Show resolved Hide resolved
Co-authored-by: Vyas Ramasubramani <vyas.ramasubramani@gmail.com>
@vyasr vyasr self-requested a review August 5, 2021 18:03
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.

Misread the test input, this looks good to go.

@galipremsagar galipremsagar added 5 - Ready to Merge Testing and reviews complete, ready to merge and removed 3 - Ready for Review Ready for review by team 4 - Needs cuDF (Python) Reviewer labels Aug 5, 2021
@codecov
Copy link

codecov bot commented Aug 5, 2021

Codecov Report

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

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

@@               Coverage Diff               @@
##             branch-21.10    #8970   +/-   ##
===============================================
  Coverage                ?   10.59%           
===============================================
  Files                   ?      116           
  Lines                   ?    19031           
  Branches                ?        0           
===============================================
  Hits                    ?     2017           
  Misses                  ?    17014           
  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 29b5f9a...4d2c0e9. Read the comment docs.

@galipremsagar
Copy link
Contributor Author

@gpucibot merge

@rapids-bot rapids-bot bot merged commit db63f61 into rapidsai:branch-21.10 Aug 5, 2021
galipremsagar added a commit to galipremsagar/cudf that referenced this pull request Aug 6, 2021
Fixes: rapidsai#6872 

In cudf, we have been concatenating a collection of `RangeIndex`'s by materializing each one of them, but instead we should rather be introspecting each RangeIndex to decide whether to materialize of not. This PR fixes it.

<!--

Thank you for contributing to cuDF :)

Here are some guidelines to help the review process go smoothly.

1. Please write a description in this text box of the changes that are being
   made.

2. Please ensure that you have written units tests for the changes made/features
   added.

3. There are CI checks in place to enforce that committed code follows our style
   and syntax standards. Please see our contribution guide in `CONTRIBUTING.MD`
   in the project root for more information about the checks we perform and how
   you can run them locally.

4. If you are closing an issue please use one of the automatic closing words as
   noted here: https://help.github.com/articles/closing-issues-using-keywords/

5. If your pull request is not ready for review but you want to make use of the
   continuous integration testing facilities please mark your pull request as Draft.
   https://docs.github.com/en/github/collaborating-with-issues-and-pull-requests/changing-the-stage-of-a-pull-request#converting-a-pull-request-to-a-draft

6. If your pull request is ready to be reviewed without requiring additional
   work on top of it, then remove it from "Draft" and make it "Ready for Review".
   https://docs.github.com/en/github/collaborating-with-issues-and-pull-requests/changing-the-stage-of-a-pull-request#marking-a-pull-request-as-ready-for-review

   If assistance is required to complete the functionality, for example when the
   C/C++ code of a feature is complete but Python bindings are still required,
   then add the label `help wanted` so that others can triage and assist.
   The additional changes then can be implemented on top of the same PR.
   If the assistance is done by members of the rapidsAI team, then no
   additional actions are required by the creator of the original PR for this,
   otherwise the original author of the PR needs to give permission to the
   person(s) assisting to commit to their personal fork of the project. If that
   doesn't happen then a new PR based on the code of the original PR can be
   opened by the person assisting, which then will be the PR that will be
   merged.

7. Once all work has been done and review has taken place please do not add
   features or make changes out of the scope of those requested by the reviewer
   (doing this just add delays as already reviewed code ends up having to be
   re-reviewed/it is hard to tell what is new etc!). Further, please do not
   rebase your branch on the target branch, force push, or rewrite history.
   Doing any of these causes the context of any comments made by reviewers to be lost.
   If conflicts occur against the target branch they should be resolved by
   merging the target branch into the branch used for making the pull request.

Many thanks in advance for your cooperation!

-->

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

Approvers:
  - Benjamin Zaitlen (https://github.com/quasiben)
  - Vyas Ramasubramani (https://github.com/vyasr)

URL: rapidsai#8970
raydouglass pushed a commit that referenced this pull request Aug 6, 2021
shwina pushed a commit to shwina/cudf that referenced this pull request Aug 9, 2021
Fixes: rapidsai#6872 

In cudf, we have been concatenating a collection of `RangeIndex`'s by materializing each one of them, but instead we should rather be introspecting each RangeIndex to decide whether to materialize of not. This PR fixes it.

<!--

Thank you for contributing to cuDF :)

Here are some guidelines to help the review process go smoothly.

1. Please write a description in this text box of the changes that are being
   made.

2. Please ensure that you have written units tests for the changes made/features
   added.

3. There are CI checks in place to enforce that committed code follows our style
   and syntax standards. Please see our contribution guide in `CONTRIBUTING.MD`
   in the project root for more information about the checks we perform and how
   you can run them locally.

4. If you are closing an issue please use one of the automatic closing words as
   noted here: https://help.github.com/articles/closing-issues-using-keywords/

5. If your pull request is not ready for review but you want to make use of the
   continuous integration testing facilities please mark your pull request as Draft.
   https://docs.github.com/en/github/collaborating-with-issues-and-pull-requests/changing-the-stage-of-a-pull-request#converting-a-pull-request-to-a-draft

6. If your pull request is ready to be reviewed without requiring additional
   work on top of it, then remove it from "Draft" and make it "Ready for Review".
   https://docs.github.com/en/github/collaborating-with-issues-and-pull-requests/changing-the-stage-of-a-pull-request#marking-a-pull-request-as-ready-for-review

   If assistance is required to complete the functionality, for example when the
   C/C++ code of a feature is complete but Python bindings are still required,
   then add the label `help wanted` so that others can triage and assist.
   The additional changes then can be implemented on top of the same PR.
   If the assistance is done by members of the rapidsAI team, then no
   additional actions are required by the creator of the original PR for this,
   otherwise the original author of the PR needs to give permission to the
   person(s) assisting to commit to their personal fork of the project. If that
   doesn't happen then a new PR based on the code of the original PR can be
   opened by the person assisting, which then will be the PR that will be
   merged.

7. Once all work has been done and review has taken place please do not add
   features or make changes out of the scope of those requested by the reviewer
   (doing this just add delays as already reviewed code ends up having to be
   re-reviewed/it is hard to tell what is new etc!). Further, please do not
   rebase your branch on the target branch, force push, or rewrite history.
   Doing any of these causes the context of any comments made by reviewers to be lost.
   If conflicts occur against the target branch they should be resolved by
   merging the target branch into the branch used for making the pull request.

Many thanks in advance for your cooperation!

-->

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

Approvers:
  - Benjamin Zaitlen (https://github.com/quasiben)
  - Vyas Ramasubramani (https://github.com/vyasr)

URL: rapidsai#8970
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
5 - Ready to Merge Testing and reviews complete, ready to merge bug Something isn't working non-breaking Non-breaking change Python Affects Python cuDF API.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] cudf concat / append must not materialize contiguous rangeIndex objects
3 participants