-
-
Notifications
You must be signed in to change notification settings - Fork 17.9k
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
REF: Move pandas.core.categorical #19269
REF: Move pandas.core.categorical #19269
Conversation
""" | ||
try: | ||
codes = np.asarray(codes, np.int64) | ||
except (ValueError, TypeError): |
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.
Note: I changed this from except Exception
for the linter.
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.
+1
pandas/core/categorical.py
Outdated
msg = self._get_repr(length=False, footer=True).replace("\n", ", ") | ||
result = ('[], {repr_msg}'.format(repr_msg=msg)) | ||
|
||
return re |
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.
Is this unavoidable for older pickle files?
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.
not sure what you are pointing to. but you need to have an entry in pandas.compat/pickle_compat.py
to handle the pickle compat
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.
Thanks. I thought we had something like that.
There sure are a lot of people importing I think I'll add a small future warning saying to use the public API. Edit: oh A good chunk of that is people just uploading their entire site-packages to github :/ |
you could add a deprecating module (e.g. like tslib.py) in the to-level to make a nice transition |
This looks pretty benign to me (I'm assuming there weren't any clandestine changes in the cut/paste). I'd argue for calling it |
you prob need to change the paths in api.rst as well |
no this is naming scheme is well-established |
api.rst is all good. All the references via the public API. |
Codecov Report
@@ Coverage Diff @@
## master #19269 +/- ##
=========================================
Coverage ? 91.5%
=========================================
Files ? 150
Lines ? 48879
Branches ? 0
=========================================
Hits ? 44729
Misses ? 4150
Partials ? 0
Continue to review full report at Codecov.
|
not sure if you saw my comment above. I think you need a dummpy |
Yeah I have one. The diff is strange. The warning is at the top, and the import is at the bottom. |
I c, ok. can you add a test for this (maybe in test_api), and add to the deprecations list. otherwise lgtm. |
c214234
to
6fd24d8
Compare
Added a test. |
Moved pandas.core.categorical to arrays.
6fd24d8
to
70da106
Compare
Are we cool with merging PRs before the Mac build finishes? Travis had another outage, so they're backlogged again. |
I think I'm going to merge PRs that are unlikely to have issues specific to the Mac build. https://travis-ci.org/pandas-dev/pandas/builds/330306709 started 2.5 hours ago, and the Mac build hasn't been scheduled yet. I'll do cleanup if I do break anything :) |
Prep for #19268