Skip to content
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

DEPR: Index.ravel returning an ndarray #36900

Merged
merged 4 commits into from Oct 6, 2020

Conversation

jbrockmendel
Copy link
Member

Make Index.ravel() behavior match every other ravel() method in existence.

@jreback jreback added Deprecate Functionality to remove in pandas Index Related to the Index class or subclasses Reshaping Concat, Merge/Join, Stack/Unstack, Explode labels Oct 6, 2020
@jreback jreback added this to the 1.2 milestone Oct 6, 2020
@jreback jreback merged commit 2272364 into pandas-dev:master Oct 6, 2020
@jreback
Copy link
Contributor

jreback commented Oct 6, 2020

nice!

@jbrockmendel jbrockmendel deleted the depr-index-ravel branch October 6, 2020 15:01
@jorisvandenbossche
Copy link
Member

jorisvandenbossche commented Oct 12, 2020

@jbrockmendel I don't think there was consensus on changing ravel to return an Index. See the original discussion at #19956, and you actually did a similar PR before (but to return the EA, #32641) where I said the same.

  • The Index method will now be inconsistent with the Series method
  • What's the use case of actually needing an Index.ravel method?

@jbrockmendel
Copy link
Member Author

The Index method will now be inconsistent with the Series method

I would be OK changing the deprecation so to say the future behavior is to match self._values.ravel, matching Series behavior.

What's the use case of actually needing an Index.ravel method?

The same as the use case for Series.ravel: you want to write code that is ndarray/index/series-agnostic

jbrockmendel added a commit to jbrockmendel/pandas that referenced this pull request Oct 13, 2020
@jorisvandenbossche
Copy link
Member

I would be OK changing the deprecation so to say the future behavior is to match self._values.ravel, matching Series behavior.

IMO that sounds as a better option. Having consistency between Series and Index here would be good I think (although Index is more array-like than Series ..)

I am still skeptical about the need for this method at all, though (you can also do a ndim check if you eg want to ensure the array-like is 1D or converted to 1D, see eg Tom's comment at #19956 (comment))

@simonjayhawkins
Copy link
Member

I am still skeptical about the need for this method at all

as there is a top level numpy function for this https://numpy.org/doc/stable/reference/generated/numpy.ravel.html.

In the future, with __array_function__ implemented, could allow much simplification of the pandas API.

so I think OK to deprecate the method, in favour of np.ravel() support.

>>> pd.__version__
'1.2.0.dev0+765.g401d7a142c'
>>>
>>> idx = pd.Index([1, 2, 3])
>>>
>>> np.ravel(idx)
array([1, 2, 3], dtype=int64)
>>>
>>> type(_)
<class 'numpy.ndarray'>
>>>

so we could use np.ravel to get the current behaviour, of implement __array_function__ if we wanted to return a Index instead.

@jorisvandenbossche
Copy link
Member

jorisvandenbossche commented Oct 14, 2020

Also for np.ravel support through __array_function__, we would need to decide on the return value, though (so I think a large part of the discussion applies to that as well).

Because np.ravel(index) doesn't necessarily (technically) need to return an ndarray, it could also return an Index (as the warning in this PR proposes) or an EA.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Deprecate Functionality to remove in pandas Index Related to the Index class or subclasses Reshaping Concat, Merge/Join, Stack/Unstack, Explode
Projects
None yet
Development

Successfully merging this pull request may close these issues.

PeriodIndex.ravel returns ndarray[int64]
4 participants