-
-
Notifications
You must be signed in to change notification settings - Fork 18.5k
COMPAT: Categorical Subclassing #13827
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
Current coverage is 85.28% (diff: 94.73%)@@ master #13827 diff @@
==========================================
Files 139 139
Lines 50057 50062 +5
Methods 0 0
Messages 0 0
Branches 0 0
==========================================
+ Hits 42692 42695 +3
- Misses 7365 7367 +2
Partials 0 0
|
The changes in this PR are fine, but I'm not sure it's a good idea to subclassing Categorical in the API, though I would guess nothing major is likely to change before v2.0. |
It may better to discuss in #8640, I' m thinking the following stricture:
I don't think we have to wait for 2.0 as long as it does't break existing user's code, but it also should be discussed separatelly (in ML). |
@@ -330,9 +330,9 @@ def __init__(self, values, categories=None, ordered=False, | |||
|
|||
def copy(self): | |||
""" Copy constructor. """ | |||
return Categorical(values=self._codes.copy(), |
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.
maybe make a _constructor
as that is the patter we are using for other classes
1a08cf9
to
55a195b
Compare
Updated to use |
55a195b
to
13c456c
Compare
ty! |
git diff upstream/master | flake8 --diff
It allows to subclass
Categorical
related to #8640. Shoud it use_simple_new
or_constructor
?