-
-
Couldn't load subscription status.
- Fork 19.2k
DEPR: GH10623 remove items from msgpack.encode for blocks #12129
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
|
hmm legacy file for 0.17.1 not passing. I think it's tz-related. Will check later. |
|
passing now. had to make changes for |
pandas/io/packers.py
Outdated
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.
this is way hacky. you shouldn't need to specifically do this (test for DatetimeIndex. It should just be converted normally. The entire difference here is the dispatch on the dtype, which in this case would be a string.
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.
if it gets converted to a numpy array now, wouldn't the whole encode/decode for DatetimeIndex be skipped? Or is there a way to construct DatetimeTZBlock from numpy arrays?
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.
actually I think you could trivially change core/internals.py/make_block to take a DatetimeTZDtype and correctly create it. Then you would have to make to serialize that properly (as a string is prob ok) I think (you would need to fix the reverse lookup as it only deals with numpy atm).
I think the same problem exists with categorical, prob not testing it.
d1f9ed9 to
14c34c1
Compare
|
updated |
pandas/core/internals.py
Outdated
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.
you should do this first, then you can coerce if you have a dtype
|
updated... but I forgot to squash :( will do that later |
pandas/io/packers.py
Outdated
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.
I get that you have to do this, we prob need something like (in core/common.py)
def pandas_dtype(dtype):
if isinstance(dtype, compat.string_types):
try:
return DatetimeTZDtype.construct_from_string(dtype)
except TypeError:
pass
try:
return CategoricalDtype.construct_from_string(dtype)
except TypeError:
pass
dtype = np.dtype(dtype)
return dtype
|
pls add a whatsnew as well (put in API changes). |
|
this would close #12142 as well? (should add a warning in the docs about this) (e.g. show your matrix) |
|
I can change all the keys to unicode, and then it will. will update tonight |
|
Ah half way through but have to go. I will get it done tonight but is that too late? :( |
|
sure no prob |
|
updated. manually tested with python 2.7 and 3.5 on linux (zlib and no compress). |
pandas/io/packers.py
Outdated
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.
just use u() from compat/__init__.py (and amend with this).
|
updated. So this is how I tested P2 & P3 compatibility: use the script to generate legacy data for 0.18.0 in P2 and P3, put them in 0.18.0 folder, and let the tests pick them up. |
|
thanks! I added some more test msgpacks for 0.17.1 to cover the bases |
|
https://travis-ci.org/pydata/pandas/jobs/109620001 I think we need to skip if blosc is not installed on these tests. can you do a PR for this? |
closes #10623