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.

Sign up for GitHub

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

Jump to bottom

BaseIndex and IndexedFrame has overlapping logics #10068

Open
isVoid opened this issue Jan 18, 2022 · 4 comments
Open

BaseIndex and IndexedFrame has overlapping logics #10068

isVoid opened this issue Jan 18, 2022 · 4 comments
Assignees
@isVoid
Labels
feature request New feature or request improvement Improvement / enhancement to an existing function Python Affects Python cuDF API.

Comments

@isVoid
Copy link
Contributor

isVoid commented Jan 18, 2022

From review comments: #9832 (review), recent python refactors #9832 #9807 has created some duplicate logics in BaseIndex and IndexedFrame. The main diverging part of the implementation mostly lies in column selection (handling index columns and user inputs). For some methods (such as _apply_boolean_mask), which unconditionally pass in all columns for index/indexed_frame, we should be able to merge common code path while dispatching just the column selection part to each object to reduce code duplication.
@isVoid isVoid added feature request New feature or request Needs Triage Need team to review and classify labels Jan 18, 2022
@isVoid isVoid self-assigned this Jan 18, 2022
@isVoid isVoid added improvement Improvement / enhancement to an existing function code quality labels Jan 18, 2022
@isVoid isVoid added the Python Affects Python cuDF API. label Jan 18, 2022
@github-actions
Copy link

This issue has been labeled inactive-30d due to no recent activity in the past 30 days. Please close this issue if no further response or action is needed. Otherwise, please respond with a comment indicating any updates or changes to the original issue and/or confirm this issue still needs to be addressed. This issue will be labeled inactive-90d if there is no activity in the next 60 days.

@isVoid isVoid mentioned this issue Mar 1, 2022
Merged
rapids-bot bot pushed a commit that referenced this issue Mar 18, 2022
@isVoid
Refactor filling.repeat API (#10371)
47d16cb 
Part of #10153 

This PR refactors `filling.repeat` cython API to accept a list of columns instead of Frame object. In this PR I'm also trying out a possibly better pattern for index and indexed_frame to share logics, which might become a solution for #10068.

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

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

URL: #10371
@github-actions
Copy link

This issue has been labeled inactive-90d due to no recent activity in the past 90 days. Please close this issue if no further response or action is needed. Otherwise, please respond with a comment indicating any updates or changes to the original issue and/or confirm this issue still needs to be addressed.

@GregoryKimball GregoryKimball removed the Needs Triage Need team to review and classify label Jun 27, 2022
@vyasr
Copy link
Contributor

vyasr commented Jul 13, 2022

@charlesbluca @shwina I think this issue is basically stating what we discussed on a recent PR, the fact that we may eventually want a common parent class for Frame and BaseIndex to share the logic for various functions.

@shwina
Copy link
Contributor

shwina commented Jul 14, 2022

One idea @vyasr and I discussed offline is for Index to (once again) inherit from Frame. This can be done by writing a new RangeColumn class that inherits from Column. Operations on RangeColumn that require it to be materialized will return a NumericalColumn[int64].

A RangeIndex would then be a SingleColumnFrame composed of a RangeColumn, eliminating the need for a separate hierarchy for indexes.

@vyasr vyasr added this to cuDF Python Nov 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@isVoid isVoid

Labels
feature request New feature or request improvement Improvement / enhancement to an existing function Python Affects Python cuDF API.
Projects
cuDF Python
Status: Todo
Milestone
No milestone
Development

No branches or pull requests
4 participants
@vyasr @shwina @GregoryKimball @isVoid