-
-
Notifications
You must be signed in to change notification settings - Fork 18.4k
Allow dtype='category' or dtype=None for Series with Categorical #12636
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
Changes from 4 commits
9bfd321
d7f927f
ed42b87
fd07315
ca1b9e7
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -32,6 +32,7 @@ | |
from pandas.core import generic, base | ||
from pandas.core.internals import SingleBlockManager | ||
from pandas.core.categorical import Categorical, CategoricalAccessor | ||
from pandas.core.dtypes import CategoricalDtype | ||
import pandas.core.strings as strings | ||
from pandas.tseries.common import (maybe_to_datetimelike, | ||
CombinedDatetimelikeProperties) | ||
|
@@ -195,9 +196,12 @@ def __init__(self, data=None, index=None, dtype=None, name=None, | |
else: | ||
data = data.reindex(index, copy=copy) | ||
elif isinstance(data, Categorical): | ||
if dtype is not None: | ||
# Allow dtype=category only, otherwise error | ||
if ((dtype is not None) and | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this is not right here; use is_categorical_dtype(dtype) |
||
not isinstance(dtype, CategoricalDtype)): | ||
raise ValueError("cannot specify a dtype with a " | ||
"Categorical") | ||
"Categorical unless " | ||
"dtype='category'") | ||
elif (isinstance(data, types.GeneratorType) or | ||
(compat.PY3 and isinstance(data, map))): | ||
data = list(data) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -80,6 +80,13 @@ def test_basic(self): | |
self.assertFalse(is_categorical(np.dtype('float64'))) | ||
self.assertFalse(is_categorical(1.0)) | ||
|
||
def test_series_with_dtype(self): | ||
self.assertRaises( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you add a comment like |
||
ValueError, lambda: Series(Categorical([1, 2, 3]), dtype='int64')) | ||
s = Series(Categorical([1, 2, 3]), dtype='category') | ||
self.assertTrue(is_categorical_dtype(s)) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. these go in tests/series/ test_constructor There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You mean moving the contents of this test into test_constructor_categorical() in tests/series/test_constructor? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yep |
||
self.assertTrue(is_categorical_dtype(s.dtype)) | ||
|
||
|
||
class TestDatetimeTZDtype(Base, tm.TestCase): | ||
|
||
|
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.
also need to handle a passed category dtype with a non Categorical
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.
Fixed others, this is the last one. Do you mean ensuring this is handled?
Because that seems graceful:
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.
yep - look over the test coverage for that and see if anything missing (out new tests near)
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.
Got it. Thanks. Running nosetests now and then will push.