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

API, DEPR: Raise and Deprecate Reshape for Pandas Objects #13012

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
6 participants
@gfyoung
Member

gfyoung commented Apr 27, 2016

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

View changes

pandas/core/categorical.py Outdated
@@ -358,6 +358,19 @@ def itemsize(self):
def reshape(self, new_shape, **kwargs):
""" compat with .reshape """

This comment has been minimized.

@jreback

jreback Apr 27, 2016

Contributor

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

This comment has been minimized.

@gfyoung

gfyoung Apr 28, 2016

Member

Done.

@jreback jreback added the Compat label Apr 27, 2016

@jreback jreback added this to the 0.18.1 milestone Apr 27, 2016

@jreback

This comment has been minimized.

Contributor

jreback commented Apr 27, 2016

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 comment has been minimized.

Member

shoyer commented Apr 27, 2016

This is definitely a good idea 👍

@kawochen

This comment has been minimized.

Contributor

kawochen commented Apr 27, 2016

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

@gfyoung

This comment has been minimized.

Member

gfyoung commented Apr 27, 2016

@jreback: Sure thing

@kawochen : Yikes, good catch. Will fix.

@gfyoung

This comment has been minimized.

Member

gfyoung commented Apr 28, 2016

@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

This comment has been minimized.

Contributor

jreback commented Apr 28, 2016

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

This comment has been minimized.

Member

gfyoung commented Apr 28, 2016

  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 jreback removed this from the 0.18.1 milestone Apr 28, 2016

@jreback

This comment has been minimized.

Contributor

jreback commented Apr 28, 2016

see my point above

@gfyoung

This comment has been minimized.

Member

gfyoung commented Apr 28, 2016

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

@gfyoung gfyoung changed the title from API: Prevent invalid arguments to Categorical.reshape to API: Raise on Reshape for Pandas Objects Apr 28, 2016

@jreback

This comment has been minimized.

Contributor

jreback commented Apr 28, 2016

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

@gfyoung

This comment has been minimized.

Member

gfyoung commented Apr 28, 2016

Shouldn't we do the same with Categorical then?

@gfyoung gfyoung changed the title from API: Raise on Reshape for Pandas Objects to API, DEPR: Raise and Deprecate Reshape for Pandas Objects Apr 28, 2016

@jreback

This comment has been minimized.

Contributor

jreback commented Apr 28, 2016

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

@gfyoung

This comment has been minimized.

Member

gfyoung commented Apr 28, 2016

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

@codecov-io

This comment has been minimized.

codecov-io commented Apr 28, 2016

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

This comment has been minimized.

Member

gfyoung commented Apr 29, 2016

@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 jreback added this to the 0.19.0 milestone Apr 29, 2016

@jreback

This comment has been minimized.

Contributor

jreback commented Apr 29, 2016

let's do this in 0.19.0

@jreback

This comment has been minimized.

Contributor

jreback commented May 19, 2016

I pushed a v0.19.0 whatsnew (its not visible on doc-build), but there.

@gfyoung

This comment has been minimized.

Member

gfyoung commented May 19, 2016

Thanks for letting me know! Moved whatsnew comments to that doc.

@jorisvandenbossche jorisvandenbossche modified the milestones: 0.19.0, 0.20.0 Jul 8, 2016

@jorisvandenbossche

This comment has been minimized.

Member

jorisvandenbossche commented Jul 8, 2016

@gfyoung Master is now targeted for 0.19.0? Can you rebase this (and move the whatsnew notes to the correct file)?

@gfyoung

This comment has been minimized.

Member

gfyoung commented Jul 9, 2016

@jreback , @jorisvandenbossche : rebased onto master and Travis is happy. Ready to merge if there are no other concerns.

nv.validate_reshape(args, kwargs)
# while the 'new_shape' parameter has no effect,
# we should still enforce valid shape parameters

This comment has been minimized.

@jreback

jreback Jul 9, 2016

Contributor

no need for this validation

This comment has been minimized.

@gfyoung

gfyoung Jul 9, 2016

Member

That contradicts what you said earlier here.

This comment has been minimized.

@jreback

jreback Jul 9, 2016

Contributor

a small function is fine - not 20 lines of treated code that has to be maintained

I would just pass it to numpy for validation (the actual values) instead; just don't do anything with it

This comment has been minimized.

@gfyoung

gfyoung Jul 10, 2016

Member

What do you mean pass it in through numpy for validation? They don't have a separate function for validating the shape AFAIK. The only way I could do it is by calling np.reshape(Categorical.codes, shape), but that seems a little odd.

I understand the concern about not wanting to maintain all of this validation code, so if there is no simple way to do it, I might just opt for no validation since this method is being deprecated anyways.

This comment has been minimized.

@jreback

jreback Jul 10, 2016

Contributor

why ? that's exactly what I mean

that's the only fall u need

This comment has been minimized.

@jreback

jreback Jul 10, 2016

Contributor

call

This comment has been minimized.

@gfyoung

gfyoung Jul 10, 2016

Member

Fair enough, done.

@gfyoung

This comment has been minimized.

Member

gfyoung commented Jul 10, 2016

@jreback : Made the requested changes, and Travis is still happy. Ready to merge if there are no other concerns.

@jreback

View changes

doc/source/whatsnew/v0.19.0.txt Outdated
@@ -427,6 +428,8 @@ Other API changes
Deprecations
^^^^^^^^^^^^
- ``Categorical.reshape`` has been deprecated and will raise an error in a subsequent release (:issue: `12882`)

This comment has been minimized.

@jreback

jreback Jul 10, 2016

Contributor

just say removed in a subsequent release

This comment has been minimized.

@jreback

jreback Jul 10, 2016

Contributor

no spaces between issue and the number (:issue:12882) it doesn't render correctly.

This comment has been minimized.

@gfyoung

gfyoung Jul 10, 2016

Member

Ah, right. Will fix.

@jreback

View changes

pandas/core/series.py Outdated
warnings.warn("reshape is deprecated and will raise "
"in a subsequent release", FutureWarning,
stacklevel=2)
if len(args) == 1 and hasattr(args[0], '__iter__'):

This comment has been minimized.

@jreback

jreback Jul 10, 2016

Contributor

I would do the same validation here (but pass .values)

This comment has been minimized.

@jreback

jreback Jul 10, 2016

Contributor

I see, we have this back-compat thing going. ok then.

@jreback

This comment has been minimized.

Contributor

jreback commented Jul 10, 2016

needs a rebase; couple of minor doc fixes. ping when green. (also make sure that we are not triggering the warnign in the tests at all), e.g. run the tests and see if any warnings pop up (if they do then you need to fix the test code).

@jsexauer jsexauer referenced this pull request Jul 10, 2016

Open

DEPR: deprecations from prior versions #6581

0 of 85 tasks complete
@jorisvandenbossche

View changes

doc/source/whatsnew/v0.19.0.txt Outdated
- ``Index.reshape`` will raise a ``NotImplementedError`` exception when called (:issue: `12882`)
- ``Float64Index.astype(int)`` will now raise ``ValueError`` if ``Float64Index`` contains ``NaN`` values (:issue:`13149`)
- ``TimedeltaIndex.astype(int)`` and ``DatetimeIndex.astype(int)`` will now return ``Int64Index`` instead of ``np.array`` (:issue:`13209`)

This comment has been minimized.

@jorisvandenbossche

jorisvandenbossche Jul 11, 2016

Member

I think some of those entries are slipped in by rebasing?

This comment has been minimized.

@gfyoung

gfyoung Jul 11, 2016

Member

Good catch. Will fix.

@jorisvandenbossche

View changes

pandas/core/categorical.py Outdated
by `new_shape` must be the same as that of the original
shape of the `Categorical`. One shape dimension can be
negative. In this case, the value is inferred from the
length of the array and the remaining dimensions.

This comment has been minimized.

@jorisvandenbossche

jorisvandenbossche Jul 11, 2016

Member

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

This comment has been minimized.

@jreback

jreback Jul 11, 2016

Contributor

yeah, would make these really simple.

This comment has been minimized.

@gfyoung

gfyoung Jul 11, 2016

Member

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

This comment has been minimized.

Member

jorisvandenbossche commented Jul 11, 2016

@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

This comment has been minimized.

Contributor

jreback commented Jul 11, 2016

@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

This comment has been minimized.

Member

gfyoung commented Jul 12, 2016

@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

This comment has been minimized.

Member

jorisvandenbossche commented Jul 12, 2016

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

@gfyoung

This comment has been minimized.

Member

gfyoung commented Jul 12, 2016

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

@jreback

View changes

pandas/core/internals.py Outdated
# and Series and will be removed in a subsequent version,
# hence the two checks done before calling 'reshape'
if isinstance(value, ABCSeries):
value = value._values

This comment has been minimized.

@jreback

jreback Jul 13, 2016

Contributor

what is this for?

This comment has been minimized.

@jreback

jreback Jul 13, 2016

Contributor

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

This comment has been minimized.

@gfyoung

gfyoung Jul 13, 2016

Member
  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.

This comment has been minimized.

@jreback

jreback Jul 13, 2016

Contributor

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

This comment has been minimized.

@gfyoung

gfyoung Jul 13, 2016

Member

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

This comment has been minimized.

@gfyoung

gfyoung Jul 13, 2016

Member

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.

This comment has been minimized.

@jreback

jreback Jul 13, 2016

Contributor

u can also put in internals

This comment has been minimized.

@gfyoung

gfyoung Jul 13, 2016

Member

Sounds good. Done.

@jreback

View changes

pandas/io/packers.py Outdated
dtype_for(b[u'dtype']),
b[u'compress'])
# gh-13012: 'reshape' is deprecated for Categorical

This comment has been minimized.

@jreback

jreback Jul 13, 2016

Contributor

use here as well

This comment has been minimized.

@gfyoung

gfyoung Jul 13, 2016

Member

Done.

@jorisvandenbossche

View changes

pandas/indexes/base.py Outdated
@@ -944,6 +944,16 @@ def rename(self, name, inplace=False):
"""
return self.set_names([name], inplace=inplace)
def reshape(self, *args, **kwargs):
"""
Reshape an Index.

This comment has been minimized.

@jorisvandenbossche

jorisvandenbossche Jul 13, 2016

Member

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

This comment has been minimized.

@gfyoung

gfyoung Jul 13, 2016

Member

Will do as soon as Travis gives a green light.

@jreback

View changes

doc/source/whatsnew/v0.19.0.txt Outdated
.. _whatsnew_0190.api.other:
Other API changes
^^^^^^^^^^^^^^^^^

This comment has been minimized.

@jreback

jreback Jul 13, 2016

Contributor

we already have an API change section

This comment has been minimized.

@gfyoung

gfyoung Jul 13, 2016

Member

Right, done.

@jreback

View changes

doc/source/whatsnew/v0.19.0.txt Outdated
.. _whatsnew_0190.deprecations:
Deprecations
^^^^^^^^^^^^
- ``Categorical.reshape`` has been deprecated and will be removed in subsequent release (:issue:`12882`)
- ``Series.reshape`` has been deprecated and will be removed in subsequent release (:issue:`12882`)

This comment has been minimized.

@jreback

jreback Jul 13, 2016

Contributor

a subsequent release

This comment has been minimized.

@gfyoung

gfyoung Jul 13, 2016

Member

Done.

@@ -4667,6 +4667,28 @@ def rrenamer(x):
_transform_index(right, rrenamer))
def _safe_reshape(arr, new_shape):
"""
If possible, reshape `arr` to have shape `new_shape`,

This comment has been minimized.

@jreback

jreback Jul 13, 2016

Contributor

ok

@jreback

This comment has been minimized.

Contributor

jreback commented Jul 13, 2016

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

@gfyoung

This comment has been minimized.

Member

gfyoung commented Jul 14, 2016

@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 jreback closed this in 084ceae Jul 14, 2016

@jreback

This comment has been minimized.

Contributor

jreback commented Jul 14, 2016

thanks @gfyoung

@gfyoung gfyoung deleted the forking-repos:categorical-reshape-validate branch Jul 14, 2016

gfyoung added a commit to forking-repos/pandas that referenced this pull request Dec 27, 2017

CLN: Drop the .reshape method from classes
Remove the method for Series, Categorical,
and Index. Deprecated or errored in v0.19.0

xref pandas-devgh-13012

@jreback jreback referenced this pull request Dec 27, 2017

Open

DEPR: deprecations log for removed issues #13777

115 of 115 tasks complete

jreback added a commit that referenced this pull request Dec 27, 2017

CLN: Drop the .reshape method from classes (#18954)
Remove the method for Series, Categorical,
and Index. Deprecated or errored in v0.19.0

xref gh-13012

hexgnu added a commit to hexgnu/pandas that referenced this pull request Dec 28, 2017

CLN: Drop the .reshape method from classes (pandas-dev#18954)
Remove the method for Series, Categorical,
and Index. Deprecated or errored in v0.19.0

xref pandas-devgh-13012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment