API, DEPR: Raise and Deprecate Reshape for Pandas Objects

[gfyoung]

reshape largely exists for compat reasons with numpy but in the interests of moving away from that, this PR will cause objects like Series, Index, and Categorical to deprecate OR raise when such a method is called.

[jreback]

can you make this a more propert doc-string (e.g. Parameters)

[gfyoung]

Done.

[jreback]

need these types of checks for Index as well. might as well combine the checks for Categorical,Index,Series (e.g. make a method to do it in core/base)

[shoyer]

This is definitely a good idea 👍

[kawochen]

had a glance. don't think you are handling (-2, -1)?

[gfyoung]

@jreback: Sure thing

@kawochen : Yikes, good catch. Will fix.

[gfyoung]

@jreback : Actually, on second thought:

1. Index has no reshape method --> but is this PR the right place to add such a method (is it really necessary even?)

2. Series may not need such checks IMO because it passes new_shape to its _values attribute for reshaping. AFAICT that _values attribute is either ndarray (or sub-class) OR Categorical. Seems like duplicate work to have it validate in Series and then have to validate again in _values. Thoughts?

[jreback]

Index should have one (that should just raise), as will Series in the next release. So this is just a bandaid. Categorical should raise as well, its an unsupported method.

[gfyoung]

1. The raising should be for a separate PR, right?
2. My point was that I saw no reason to perform validation twice given what _values can be. In light of your first point, I do agree that this is only a band-aid at best.

[jreback]

see my point above

[gfyoung]

Ah, okay, I see. Let them all raise then. Should be relatively straightforward.

[jreback]

@gfyoung but Series you can't change now as its technically an API change. however, would be ok with deprecating it now.

[gfyoung]

Shouldn't we do the same with Categorical then?

[jreback]

yes, didn't realize that .reshape was already defined.

[gfyoung]

Alrighty, so I'll deprecate for both Series and Categorical and add a method for Index that raises.

[codecov-io]

Current coverage is 84.38%

Merging #13012 into master will increase coverage by <.01%

@@ master #13012 diff @@ ========================================== Files 142 142 Lines 51225 51235 +10 Methods 0 0 Messages 0 0 Branches 0 0 ========================================== + Hits 43225 43234 +9  - Misses 8000 8001 +1  Partials 0 0 

Powered by Codecov. Last updated by 20de266...3ad161d

[gfyoung]

@jreback : I've added deprecation and raising behaviour for Series, Index, and Categorical. Travis gives a green light, so this is ready to merge if there is nothing else.

[jreback]

let's do this in 0.19.0

[jorisvandenbossche]

Is it needed to add this documentation on new_shape if we are deprecating its usage?

[jreback]

yeah, would make these really simple.

[gfyoung]

As this is really a numpy thing, I'm just going to point the documentation on new_shape to np.reshape (we're deprecating this function after all, so I don't feel too bad for tying it to numpy).

[jorisvandenbossche]

@jreback What is the reason of adding a Index.reshape that raises? That seems unnecessary to me (and only adds to the list of Index methods).

For people using Series.reshape (not sure why they would do it, but probably it is used in the wild), is Series.values.reshape the alternative they should use instead?
If so, maybe add to the depr message?

[jreback]

@jorisvandenbossche we dont technically need this on Index as the idea is to raise on Series/Categorical in the future. (it should have been added originally to Index).

[gfyoung]

@jreback , @jorisvandenbossche : Made requested doc changes (no need to run Travis on those, hence the [ci skip]), so ready to merge if there are no other concerns.

[jorisvandenbossche]

@gfyoung See my comment from above: #13012 (comment)

[gfyoung]

@jorisvandenbossche: Ah whoops, good point. Will do.

[jreback]

what is this for?

[jreback]

oh I see.

ok, need an internal function to do this then (rather than having this duplicated code)

put in pandas.types.concat maybe compat_reshape

[gfyoung]

1. It was throwing warnings during testing.

2. The current code for Series.reshape will simply call self._values.reshape(...), so I am preempting that so we don't have to call the reshape method for Series.

[jreback]

and that's fine, except for have code duplication, so just put it in an internal function as I said above

[gfyoung]

I didn't get that second note until after I posted mine. 😄

[gfyoung]

Though why pandas.types.concat? We aren't really concatenating anything here. My first inclination was to try to put in pandas.compat, but I don't see a good home for it there.

[jreback]

u can also put in internals

[gfyoung]

Sounds good. Done.

[jorisvandenbossche]

Can you put "Not implemented" on the first line? As otherwise in the methods overview in the Index docstring, you will see the reshape method with this explanation, and from that it would seem it has a working reshape method

[gfyoung]

Will do as soon as Travis gives a green light.

[jreback]

lgtm (minor doc comments). will need a rebase after #13147

[gfyoung]

@jreback : Made the requested doc changes as well as rebased, and Travis is still passing. Ready to merge if there are no other concerns.

[jreback]

thanks @gfyoung