-
-
Notifications
You must be signed in to change notification settings - Fork 18.5k
BUG: CategoricalIndex allowed reindexing duplicate sources #28257
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
Conversation
Hello @batterseapower! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found: There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻 Comment last updated at 2019-10-11 09:47:23 UTC |
Had to fix a couple of incidental Pandas bugs that were surfaced by the main fix.
|
e2d2666
to
db711fe
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good overall.
…uplicate targets: this is the wrong way around
5a9268a
to
5d3a861
Compare
Please let me know if this needs more work.. |
lgtm. cc @jbrockmendel @jschendel if any comments. |
LGTM |
thanks @batterseapower |
For consistency with normal indexes,
CategoricalIndex.reindex
should allow targets to be duplicated but not the sources. However, currently in allows the source to be duplicated but not the targets, which is exactly the wrong behaviour.Most of the work here is fixing the tests, which in many cases explicitly check for the incorrect behaviour.
Fixes #25459
(I can't run the full testsuite on my machine but the relevant categorical tests do seem to pass. Hoping that the GitHub CI infrastructure will pick up any other failures.)