COMPAT: remove Categorical pickle compat with Pandas < 0.16 by topper-123 · Pull Request #27538 · pandas-dev/pandas (original) (raw)
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
[ Show hidden characters]({{ revealButtonHref }})
- closes #xxxx
- tests added / passed
- passes
black pandas - passes
git diff upstream/master -u -- "*.py" | flake8 --diff - whatsnew entry
Removes Categorical pickle compat with Pandas < 0.16.
I've added a categorical.pickle file to replace the deprecated file. It was create like this:
cat = pd.Categorical(['a', 'b', 'c', 'd']) pickle.dump(cat, open(, 'wb'))
Categorical Data Type
IO issues that don't fit into a more specific label
pandas objects compatability with Numpy or Python functions
labels
why close? ive been looking forward to seeing these big chunks of pickle compat code removed
Sorry, I saw there were errors, and don't have time today to figure them out and didn't want to leave a failing PR open too long.
I'll open the PR again, when I figure out the problem.
Reopened and tests passed.
| (pd.read_json, "os", ("io", "json", "data", "tsframe_v012.json")), |
|---|
| (pd.read_msgpack, "os", ("io", "msgpack", "data", "frame.mp")), |
| (pd.read_pickle, "os", ("io", "data", "categorical_0_14_1.pickle")), |
| (pd.read_pickle, "os", ("io", "data", "categorical.pickle")), |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should there be a version number on this?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, that will probably be easier for future maintenance. Will add it.
Small question, otherwise LGTM
| else: |
|---|
| state["_ordered"] = False |
| # 0.21.0 CategoricalDtype change |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why is there a 025 pickle
file included? this is covered by the legacy pickle dukes no?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test test_read_fspath_all tests if reading custom file paths (CustomFS) returns an expected value. Seems reasonable that a pickle are included for that testing. Adding 0.25 in the name is not strictly necessary, but can possibly be handy info to have.
Probably this test could instead test if temp files are read correctly back in, but that would be for another PR, IMO.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we already have round trip categorical pickle tests
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Didn't mean test round-tripping, but use roundtripping in this test, rather than keeping a file in /data.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm. I don't have a problem with the inclusion of the 0.25 file but am indifferent if we want to do away with it as well
| - :meth:`pandas.Series.str.cat` does not accept list-likes *within* list-likes anymore (:issue:`27611`) |
|---|
| - Removed the previously deprecated :meth:`ExtensionArray._formatting_values`. Use :attr:`ExtensionArray._formatter` instead. (:issue:`23601`) |
| - Removed the previously deprecated ``IntervalIndex.from_intervals`` in favor of the :class:`IntervalIndex` constructor (:issue:`19263`) |
| - Ability to read pickles containing :class:`Categorical` instances created with pre-0.16 version of pandas has been removed (:issue:`27538`) |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The whatsnew for v0.25.0 already states that we dropped pickle support prior to 0.20.3 so this isn't necessary, though I guess doesn't hurt to include
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO it is ok to include this, given that something won't work that previously did work...
@topper-123 can you merge master on this to be safe? If no other feedback in 2 days let's get this merged
There are other <0.16 pickle tests out there. Can we get rid of those too? tests.tseries.test_offsets.test_pickle_v0_15_2 is a blocker for cythonizing DateOffset.
I think so - we are only guaranteed backwards compatible to pandas 0.20.3 as of the 0.25 release
see #27082
proost pushed a commit to proost/pandas that referenced this pull request
proost pushed a commit to proost/pandas that referenced this pull request
Labels
Categorical Data Type
pandas objects compatability with Numpy or Python functions
IO issues that don't fit into a more specific label