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

ENH: add na_action to Categorical.map & CategoricalIndex.map #51645

Merged
merged 16 commits into from
Mar 30, 2023

Conversation

topper-123
Copy link
Contributor

@topper-123 topper-123 mentioned this pull request Mar 6, 2023
1 task
@topper-123 topper-123 force-pushed the categorical_map_na_Action branch 2 times, most recently from 6b461b2 to 64b7297 Compare March 14, 2023 18:44
@mroeschke mroeschke added the Categorical Categorical Data Type label Mar 17, 2023
.pre-commit-config.yaml Outdated Show resolved Hide resolved
@topper-123
Copy link
Contributor Author

I've updated again. I noticed that the implicit default for na_action is "ignore" for Categorical.map, while it's None generally. This is a bit of an issue because e.g. Series.map has a default of None and that can't be changed, so there's some mismatch when calling the Categorical directly compared to doing it through Series.map.

So I've set the default value of na_action="ignore" in Categorical.map as being deprecated and set to be changed in the next version.

@topper-123
Copy link
Contributor Author

I’ve simplified the implementation.

@topper-123
Copy link
Contributor Author

Ping.

mapped = obj._map_values(mapper=f, convert=self.convert_dtype)
# apply doesn't have a `na_action` keyword and for backward compat reasons
# we need to give `na_action="ignore"` for categorical data.
# TODO: remove the `na_action="ignore"` has been removed from Categorical.
Copy link
Member

Choose a reason for hiding this comment

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

Is this supposed to refer to once the deprecation is enforced?


.. deprecated:: 2.1.0

The dault value of 'ignore' has been deprecated and will be changed to
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
The dault value of 'ignore' has been deprecated and will be changed to
The default value of 'ignore' has been deprecated and will be changed to

Copy link
Member

@mroeschke mroeschke left a comment

Choose a reason for hiding this comment

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

The logic looks a lot simpler thanks. Kinda unfortunate that user will automatically get a deprecation warning, but i think this is okay as map is a fairly uncommon method

@mroeschke mroeschke added this to the 2.1 milestone Mar 30, 2023
@mroeschke mroeschke merged commit 72cd001 into pandas-dev:main Mar 30, 2023
@mroeschke
Copy link
Member

Thanks @topper-123

@topper-123 topper-123 deleted the categorical_map_na_Action branch March 30, 2023 16:29
topper-123 added a commit to topper-123/pandas that referenced this pull request Apr 1, 2023
…dev#51645)

* ENH: add na_action to Categorical.map

* add GH numbers

* pre-commit issues

* map Categorical with Series

* REF: simplify .map

* pass test_map

* fix whatsnew

* cleanups

* pre-commit

* deprecate Categorical.map(na_action=ignore)

* fix docstrings

* fix rebase

* simplity implementation

* fix warn

* fix comments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Categorical Categorical Data Type
Projects
None yet
2 participants