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.get_value ? #19728

Closed
jorisvandenbossche opened this issue Feb 16, 2018 · 9 comments · Fixed by #33907
Closed

DEPR: Index.get_value ? #19728

jorisvandenbossche opened this issue Feb 16, 2018 · 9 comments · Fixed by #33907
Labels
Deprecate Functionality to remove in pandas Indexing Related to indexing on series/frames, not to indexes themselves
Projects
Milestone

Comments

@jorisvandenbossche
Copy link
Member

Index.get_value is a very strange (from user perspective), and completely undocumented method, so let's rename it to _get_value for internal usage and deprecate the public one?

Example usage to show its 'not-usefulness':

In [85]: idx = pd.Index([1, 2, 3])

In [87]: idx.get_value(np.array([0, 1, 2]), 2)
Out[87]: 1

(so you can index into another object based on the location of the key in the calling index)

@jreback
Copy link
Contributor

jreback commented Feb 16, 2018

iirc this is heavily used in indexing code
renaming just causes bloat - rather deprecation and fixing are better

@jorisvandenbossche
Copy link
Member Author

But what needs to be fixed? If it is heavily used in indexing code, it is probably useful?
IMO, the only thing that needs to be fixed is the fact that it is a public method, and then simply renaming does the trick (indexing code can maybe use a refactor, but that's a totally different and much bigger issue)

@jreback
Copy link
Contributor

jreback commented Feb 16, 2018

no you are missing the point here. A lot of index methods are used internally to do indexing. But that does not necessarily mean they should be publicly exposed, though they are currently. If we are making first class extensions, then all methods should be public and pandas itself should not even internally rely on indexing methods (and if they do they would be private).

So its public now, deprecating the current function just causes one more method and that is just smelly. If you want to deprecate it, then it should be completely removed from the internal API and just call public methods).

@jorisvandenbossche
Copy link
Member Author

jorisvandenbossche commented Feb 16, 2018

I was not speaking about extensions here.
But about that, do you think there are Index subclasses in the wild that rely on get_value ?

@jorisvandenbossche
Copy link
Member Author

Ah, yes, based on @shoyer 's comment here #15258, it's indeed needed at this moment to implement custom Index subclass.

@jreback
Copy link
Contributor

jreback commented Feb 16, 2018

I don't care if we have Index sub-classes in the wild. that's not my point. We should get rid of .get_value, but NOT by simply renaming it. It needs to be properly removed internally.

@shoyer
Copy link
Member

shoyer commented Feb 16, 2018

I don't think there are Index subclasses in the wild that rely on get_value. There might be subclasses that implement get_value, but they should still work fine when it's removed.

@jorisvandenbossche
Copy link
Member Author

There might be subclasses that implement get_value, but they should still work fine when it's removed.

@shoyer But if Series.__getitem__ now uses its index get_value, and we would rename / remove that, doesn't that mean that a Series with such a subclassed index would no longer work for those cases where it was used?

@shoyer
Copy link
Member

shoyer commented Feb 16, 2018

If I recall correctly, index.get_value(series, key) is basically equivalent to series.iloc[index.get_loc(key)] (but it possibly faster). Assuming that the Index subclass implements get_loc (which is basically required for meaningful indexing) it should work fine with this fall-back.

@jbrockmendel jbrockmendel added the Deprecate Functionality to remove in pandas label Jul 30, 2018
@jbrockmendel jbrockmendel added the Indexing Related to indexing on series/frames, not to indexes themselves label Feb 8, 2020
@jbrockmendel jbrockmendel mentioned this issue May 1, 2020
5 tasks
@jreback jreback added this to the 1.1 milestone May 1, 2020
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 Indexing Related to indexing on series/frames, not to indexes themselves
Projects
No open projects
Indexing
Awaiting triage
Development

Successfully merging a pull request may close this issue.

4 participants