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

Various internal MultiIndex improvements #9243

Merged
merged 19 commits into from
Sep 22, 2021

Conversation

vyasr
Copy link
Contributor

@vyasr vyasr commented Sep 17, 2021

This PR is a follow-up to #9191 to further simplify the MultiIndex class. It removes various unused functions, inlines and simplifies a number of other single-use internal functions, and moves more methods to Frame that can be shared. It also makes numerous other miscellaneous improvements to the code in MultiIndex to simplify further rewrites.

@vyasr vyasr added 3 - Ready for Review Ready for review by team code quality Python Affects Python cuDF API. improvement Improvement / enhancement to an existing function breaking Breaking change labels Sep 17, 2021
@vyasr vyasr added this to the CuDF Python Refactoring milestone Sep 17, 2021
@vyasr vyasr requested a review from a team as a code owner September 17, 2021 00:20
@vyasr vyasr self-assigned this Sep 17, 2021
@vyasr
Copy link
Contributor Author

vyasr commented Sep 17, 2021

Two notes:

  • I've marked this as breaking because the labels parameter was removed from the constructor. This parameter has been deprecated for over two years and should be safe to remove at this point.
  • I've targeted this at 21.10 to take advantage of CI for the moment but we can safely retarget to 21.12 if we don't want to squeeze this in at this point. There's nothing critical in this PR.

@codecov
Copy link

codecov bot commented Sep 17, 2021

Codecov Report

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

Impacted file tree graph

@@               Coverage Diff               @@
##             branch-21.12    #9243   +/-   ##
===============================================
  Coverage                ?   10.89%           
===============================================
  Files                   ?      115           
  Lines                   ?    19087           
  Branches                ?        0           
===============================================
  Hits                    ?     2080           
  Misses                  ?    17007           
  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 079af45...7cdbe12. Read the comment docs.

Copy link
Contributor

@thomcom thomcom left a comment

Choose a reason for hiding this comment

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

A delightful stroll down memory lane. Great job digging through the miserable special cases of MultiIndex, thanks!

python/cudf/cudf/core/multiindex.py Show resolved Hide resolved
python/cudf/cudf/core/multiindex.py Outdated Show resolved Hide resolved
python/cudf/cudf/core/multiindex.py Outdated Show resolved Hide resolved
python/cudf/cudf/core/multiindex.py Show resolved Hide resolved
python/cudf/cudf/core/multiindex.py Show resolved Hide resolved
@vyasr
Copy link
Contributor Author

vyasr commented Sep 20, 2021

Temporarily leaving this PR pointing to branch-21.10 to for CI, but moving to the 21.12 milestone. Will retarget the PR once changes are finalized (or 21.12 CI is up and running).

@marlenezw
Copy link
Contributor

Overall this is looking good to me. It might be able to make it into 21.10 if there aren't any other major changes (not sure when burn down is though.)

@thomcom thomcom self-requested a review September 21, 2021 18:42
@vyasr
Copy link
Contributor Author

vyasr commented Sep 21, 2021

@galipremsagar once you're happy with this it's up to you whether you want to merge this into 21.10 or 21.12. It seems like it's basically ready, but as we discussed offline (and commented on this thread above) it may be wiser to hold off until 21.12 to avoid any last minute surprises.

@galipremsagar galipremsagar changed the base branch from branch-21.10 to branch-21.12 September 21, 2021 19:55
Copy link
Contributor

@galipremsagar galipremsagar left a comment

Choose a reason for hiding this comment

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

LGTM, lets merge this into branch-21.12 as discussed offline. Can you rebase the changes?

@vyasr vyasr requested review from a team as code owners September 21, 2021 20:01
@vyasr
Copy link
Contributor Author

vyasr commented Sep 21, 2021

Wow I completely borked the merge somehow. Going to rebase, will need to remove unnecessary requested reviewers.

@vyasr vyasr force-pushed the refactor/multiindex_part2 branch from e8fa910 to be05fa1 Compare September 21, 2021 20:09
@vyasr
Copy link
Contributor Author

vyasr commented Sep 21, 2021

Ah I think I see the problem. Has forward-merge from 21.10->21.12 not been set up yet? I think I had commits from merging 21.10 in my PR that were not yet present on 21.12.

@vyasr vyasr removed request for a team, hyperbolic2346 and codereport September 21, 2021 20:12
…th version from 21.10 to prevent forward merge issues.
@vyasr
Copy link
Contributor Author

vyasr commented Sep 21, 2021

I had fixed a typo in a test created in #9258, which led to conflicts from merges. I've removed that change entirely to avoid causing issues with the 21.10->21.12 forward merge once it's introduced.

@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 labels Sep 21, 2021
@vyasr
Copy link
Contributor Author

vyasr commented Sep 22, 2021

@gpucibot merge

@rapids-bot rapids-bot bot merged commit 4ac54e1 into rapidsai:branch-21.12 Sep 22, 2021
@vyasr vyasr deleted the refactor/multiindex_part2 branch January 14, 2022 18:00
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 breaking Breaking change improvement Improvement / enhancement to an existing function Python Affects Python cuDF API.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants