Skip to content

Use a column to store categories, rather than a mapping#69

Merged
rgommers merged 5 commits intodata-apis:mainfrom
shwina:store-categories-in-column
Jul 28, 2022
Merged

Use a column to store categories, rather than a mapping#69
rgommers merged 5 commits intodata-apis:mainfrom
shwina:store-categories-in-column

Conversation

@shwina
Copy link
Contributor

@shwina shwina commented Sep 29, 2021

This PR implements one of the changes mentioned in #41, i.e.,

Add get_children() method, and store the mapping that is now in Column.describe_categorical in a child column instead. Note that child columns are also needed for variable-length strings.

Replacing the previous mapping with a child column now implies that the data buffer stores integer indices into the child column.

@shwina shwina mentioned this pull request Sep 29, 2021
Merged
@vnlitvinov
Copy link
Contributor

vnlitvinov commented May 4, 2022

@shwina @rgommers should we maybe not try to disguise the (explicit) categories as generic "children"?

I think just replacing the type of df.describe_categorical['mapping'] from dict to Column instead of introducing of a new method could be better - it's still a mapping after all 🙃

@vnlitvinov vnlitvinov mentioned this pull request May 4, 2022
Merged
@rgommers
Copy link
Member

rgommers commented May 26, 2022

We briefly touched on this PR today, we'd like to move it forward since all interchange protocol implementers seem to be in favor of this. @vnlitvinov plans to revisit his proposal from above and add that here or open an alternative PR. Then we can try to finalize this, and then update implementations in the various libraries for it.

@shwina
Copy link
Contributor Author

shwina commented May 27, 2022

still a mapping after all

I'm not sure I quite follow; the Column is explicitly not a mapping, rather it is concretely the categories. Perhaps I'm missing something?

@vnlitvinov vnlitvinov force-pushed the store-categories-in-column branch from 91ed593 to 902ba7c Compare June 1, 2022 10:12
@vnlitvinov
Copy link
Contributor

vnlitvinov commented Jun 1, 2022

@shwina I've missed your comment, so I went ahead with the original plan we discussed last meeting and I had added a commit doing so. If you don't like the idea feel free to remove my commit.

As for it not being a mapping... it somewhat is :) it maps an integer index to a category value.

@rgommers
Copy link
Member

rgommers commented Jun 21, 2022

After discussion on Thursday, @shwina and others were happy with the changes @vnlitvinov pushed. With one change to make: rename mapping to categories, since it's no longer a real mapping (only implicitly, the keys are 0 ... n_categories) and it would help with transitioning too.

kkraus14
kkraus14 reviewed Jul 7, 2022
View reviewed changes
@rgommers
Copy link
Member

rgommers commented Jul 14, 2022

With one change to make: rename mapping to categories,

This change was made in the spec part, but not the rest of this PR. I'm inclined to push a change to pandas_implementation.py and then merge this.

@rgommers
Copy link
Member

rgommers commented Jul 14, 2022

With one change to make: rename mapping to categories,

This change was made in the spec part, but not the rest of this PR. I'm inclined to push a change to pandas_implementation.py and then merge this.

I tried this - since the tests no longer pass with this PR, and we now have an actual Pandas implementation, it may be better to completely remove this early prototype in pandas_implementation.py.

@rgommers
Copy link
Member

rgommers commented Jul 14, 2022

I tried this - since the tests no longer pass with this PR, and we now have an actual Pandas implementation, it may be better to completely remove this early prototype in pandas_implementation.py.

Discussed in today's call: everyone is happy with deleting this prototype code.

rgommers added a commit to rgommers/dataframe-api that referenced this pull request Jul 14, 2022
@rgommers
Remove prototype Pandas implementation from protocol/
81d5226 
Now that we have four real-world implementations in cuDF, Vaex,
Modin and Pandas, we no longer need this prototype. Having it
here makes it harder to merge other PRs (see data-apisgh-69), so let's
remove it now.
@rgommers rgommers mentioned this pull request Jul 14, 2022
Merged
rgommers added a commit that referenced this pull request Jul 28, 2022
@rgommers
Remove prototype Pandas implementation from protocol/ (#81)
d68dc3d 
Now that we have four real-world implementations in cuDF, Vaex,
Modin and Pandas, we no longer need this prototype. Having it
here makes it harder to merge other PRs (see gh-69), so let's
remove it now.
rgommers
rgommers approved these changes Jul 28, 2022
View reviewed changes
Copy link
Member

@rgommers rgommers left a comment

Choose a reason for hiding this comment

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

Everyone is happy and this is now a pretty small/straightforward diff. So merging. I'll follow up with all the implementers to ensure we will actually propagate these changes to all implementations.

Thanks @shwina, @vnlitvinov, @kkraus14

@rgommers rgommers merged commit ee9be04 into data-apis:main Jul 28, 2022
@rgommers rgommers added enhancement New feature or request bc-breaking A change that is not fully backwards compatible labels Jul 28, 2022
@mroeschke mroeschke mentioned this pull request Jul 28, 2022
Merged
2 tasks
@vnlitvinov vnlitvinov mentioned this pull request Aug 1, 2022
Closed
@honno honno mentioned this pull request Aug 4, 2022
Merged
@Dr-Irv Dr-Irv mentioned this pull request Sep 29, 2022
Merged
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@kkraus14 kkraus14 kkraus14 approved these changes

@vnlitvinov vnlitvinov vnlitvinov left review comments

@rgommers rgommers rgommers approved these changes

Assignees

No one assigned

Labels

bc-breaking A change that is not fully backwards compatible enhancement New feature or request interchange-protocol

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@shwina @vnlitvinov @rgommers @kkraus14