-
-
Notifications
You must be signed in to change notification settings - Fork 150
GH1383 Add support for creating Series/Index with dtype='category' #1391
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
Thank you for the proposal, it looks good to me.
|
I tried uncommenting the lines and it is raising many errors, I have not thing examples of Series[category] in the code yet. |
I think those have been commented out since they were first created. I think they can be uncommented, but they should be using |
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.
- Could you merge
main
into your branch? Resolving conflicts should be easy. - If you haven't, could you try the idea in #1391 (comment), namely changing to
Series[CategoricalDtype]
? - Otherwise it looks good to me, now that the idea of having
Series[CategoricalDtype]
has also been approved.
|
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.
s.astype("dictionary[pyarrow]")
isUnknown
, not sure if we have the correct overload for it. Also when I try to do it at run time I get aTypeError
so wondering what is the goal of having that only for type checking and not run time. Open to suggestions
I see, those commented-out tests do not make sense to me, either.
One small thing, otherwise good to go.
tests/series/test_properties.py
Outdated
|
||
def test_array_property() -> None: | ||
"""Test that Series.array returns ExtensionArray and its subclasses""" | ||
# casting due to pandas-dev/pandas-stubs#1383 |
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.
Please remove thie comment
If they don't work at runtime, then let's just delete them and not worry about it. |
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.
It looks good to me now, but you have not yet requested me to review again. Would you agree to merge @loicdiridollou ?
Yes good to merge (I often forgot to rerequest sorry) |
Thank you @loicdiridollou! I am getting familiar with the workflow, hopefully I'll be able to do the review better in the future. |
Index([1], dtype="category")
andSeries([1], dtype="category")
are mistyped #1383assert_type()
to assert the type of any return value