DEPR: GH10623 remove items from msgpack.encode for blocks #12129
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 |
| 'locs': b.mgr_locs.as_array, | ||
| 'values': convert(b.values), | ||
| 'blocks': [{'locs': b.mgr_locs.as_array, | ||
| 'values': convert_if_not_dt_index(b.values), |
There was a problem hiding this comment.
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.
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.
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.
kawochen
force-pushed
the
DEPR-10623
branch
2 times, most recently
from
d1f9ed9 to
14c34c1
Compare
|
updated |
| dtype = kwargs.pop('dtype') | ||
| if isinstance(dtype, compat.string_types): | ||
| dtype = DatetimeTZDtype.construct_from_string(dtype) | ||
| values = values.tz_localize('UTC').tz_convert(dtype.tz) | ||
|
|
||
| if not isinstance(values, self._holder): |
There was a problem hiding this comment.
you should do this first, then you can coerce if you have a dtype
|
updated... but I forgot to squash :( will do that later |
| @@ -245,6 +246,10 @@ def unconvert(values, dtype, compress=None): | |||
| if dtype == np.object_: | |||
| return np.array(values, dtype=object) | |||
|
|
|||
| if isinstance(dtype, string_types): | |||
There was a problem hiding this comment.
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). |
| @@ -265,28 +269,31 @@ def unconvert(values, dtype, compress=None): | |||
| return np.fromstring(values, dtype=dtype) | |||
|
|
|||
|
|
|||
| def _u(x): | |||
There was a problem hiding this comment.
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