CLN: Removed levels attribute from Categorical #13612
Conversation
gfyoung
changed the title
gfyoung
force-pushed
the
categorical-levels-remove
branch
from
bf2ab74
to
4f88df4
Compare
|
Looks good to me cc @JanSchulz |
jorisvandenbossche
added
Categorical
|
|
||
| # TODO: Remove after deprecation period in 2017/ after 0.18 | ||
| levels = property(fget=_get_levels, fset=_set_levels) | ||
|
|
||
| _ordered = None |
There was a problem hiding this comment.
iirc there is some unpick code that we should think about
There was a problem hiding this comment.
There was a problem hiding this comment.
IMO it would be ok to keep the old compat code in the pickle codepaths...
There was a problem hiding this comment.
Agree with @JanSchulz : I don't think we should remove that code-path right away.
Current coverage is 84.38%@@ master #13612 diff @@
==========================================
Files 142 142
Lines 51235 51223 -12
Methods 0 0
Messages 0 0
Branches 0 0
==========================================
- Hits 43235 43226 -9
+ Misses 8000 7997 -3
Partials 0 0
|
gfyoung
force-pushed
the
categorical-levels-remove
branch
from
4f88df4
to
cefc5fc
Compare
| @@ -1554,18 +1554,6 @@ def test_deprecated_labels(self): | |||
| res = cat.labels | |||
| self.assert_numpy_array_equal(res, exp) | |||
|
|
|||
| def test_deprecated_levels(self): | |||
There was a problem hiding this comment.
no, all of the legacy pickles need to be an EARLIER version of pandas that HAS the .levels in the categorical.
This is not testing anything.
There was a problem hiding this comment.
in this case create, let's do a different way. create a pickle in 0.18.1 with the levels. save and create create a specific test to read back and validate.
There was a problem hiding this comment.
-
Whoops, I'll generate new pickles then.
-
Wait, but isn't that what the
test_pickletests are for? I'm confused here...
There was a problem hiding this comment.
yes that IS what test_pickles does. However a Categorical that has a levels attribute does not exist in old pickles.
There was a problem hiding this comment.
If you look at the comments in test_categorical.py (which I now understand why they were included), you'll see that in fact the old pickles do have the levels attribute passed in, as far back as test_pickle_v0_14_1 - is a test in 0.18.1 for levels necessary then?
There was a problem hiding this comment.
@JanSchulz : I agree. @jreback , there is no need to generate pickles with the levels parameter. We already have tests for them that pass in this PR with no issues. I think this is ready to be merged then since the potential backwards compatibility issue isn't actually a problem.
There was a problem hiding this comment.
@gfyoung your comments indicate you don't understand my point. We don't have tests that have a levels attribute IN A PICKLE. Thus the tests don't test the correct thing, and you cannot assure back-compat.
There was a problem hiding this comment.
I think we do: https://github.com/pydata/pandas/blob/master/pandas/tests/data/categorical_0_14_1.pickle contains a _pickle string. Or do I misunderstand something?
There was a problem hiding this comment.
ahh @JanSchulz yes I remember now. The issue is that we don't do this with anything else, and therefore they are in the wrong place (they really should have been in the generate_data scripts). ok @gfyoung can you move those 2 test to pandas/io/read_pickle (and the associated data files). Then they will be in the correct place. We do all pickle testing in a centralized place (except for round-tripping, which doesn't require data files).
gfyoung
force-pushed
the
categorical-levels-remove
branch
3 times, most recently
from
14e2b74
to
e70c9b7
Compare
gfyoung
force-pushed
the
categorical-levels-remove
branch
from
e70c9b7
to
f1254df
Compare
|
@jreback , @JanSchulz : relocated those pickle tests for |
|
thanks! |
Deprecated back in
0.15.0and therefore long overdue. Closes #8376.