DEPR: deprecate .get_value and .set_value for Series, DataFrame, Panel & Sparse
State: closed

closes #15269


Labels:
name: Indexing
description: Related to indexing on series/frames, not to indexes themselves
name: Deprecate
description: Functionality to remove in pandas

Comments:
@TomAugspurger @jorisvandenbossche 
I am not fully convinced this is needed. As @wesm said in the issue, they were originally meant as fast access methods, and although performance has degraded over time, they are still considerably faster as any alternative.

I agree in the large majority of the use case you should not (or can avoid) having to set / get individual values, but why not keep them for those few cases you cannot otherwise?
well we have more than 1 way to do things (12 ways in fact). This is just a bad API design. Not to mention that these barely have any tests / attention, and they break in lots of cases (as they don't handle validation / dtypes propertly).

this is a no-brainer.
``.iat/.at`` are properly supported and pretty close in perf
# [Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/17739?src=pr&el=h1) Report
> Merging [#17739](https://codecov.io/gh/pandas-dev/pandas/pull/17739?src=pr&el=desc) into [master](https://codecov.io/gh/pandas-dev/pandas/commit/37860a5fc7c72a3285f060b44482d6d454fa78ae?src=pr&el=desc) will **decrease** coverage by `0.01%`.
> The diff coverage is `96.55%`.

[![Impacted file tree graph](https://codecov.io/gh/pandas-dev/pandas/pull/17739/graphs/tree.svg?token=eZ4WkYLtcO&height=150&width=650&src=pr)](https://codecov.io/gh/pandas-dev/pandas/pull/17739?src=pr&el=tree)

```diff
@@            Coverage Diff             @@
##           master   #17739      +/-   ##
==========================================
- Coverage   91.24%   91.22%   -0.02%     
==========================================
  Files         163      163              
  Lines       49918    49957      +39     
==========================================
+ Hits        45546    45574      +28     
- Misses       4372     4383      +11
```

| Flag | Coverage Δ | |
|---|---|---|
| #multiple | `89.03% <96.55%> (ø)` | :arrow_up: |
| #single | `40.25% <39.65%> (-0.06%)` | :arrow_down: |

| [Impacted Files](https://codecov.io/gh/pandas-dev/pandas/pull/17739?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pandas/core/sparse/frame.py](https://codecov.io/gh/pandas-dev/pandas/pull/17739?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvc3BhcnNlL2ZyYW1lLnB5) | `94.78% <100%> (+0.09%)` | :arrow_up: |
| [pandas/core/frame.py](https://codecov.io/gh/pandas-dev/pandas/pull/17739?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvZnJhbWUucHk=) | `97.74% <100%> (-0.09%)` | :arrow_down: |
| [pandas/core/indexing.py](https://codecov.io/gh/pandas-dev/pandas/pull/17739?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvaW5kZXhpbmcucHk=) | `93% <100%> (ø)` | :arrow_up: |
| [pandas/core/sparse/series.py](https://codecov.io/gh/pandas-dev/pandas/pull/17739?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvc3BhcnNlL3Nlcmllcy5weQ==) | `95.26% <100%> (+0.11%)` | :arrow_up: |
| [pandas/core/panel.py](https://codecov.io/gh/pandas-dev/pandas/pull/17739?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvcGFuZWwucHk=) | `96.94% <100%> (+0.03%)` | :arrow_up: |
| [pandas/core/series.py](https://codecov.io/gh/pandas-dev/pandas/pull/17739?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvc2VyaWVzLnB5) | `94.89% <77.77%> (-0.16%)` | :arrow_down: |
| [pandas/io/gbq.py](https://codecov.io/gh/pandas-dev/pandas/pull/17739?src=pr&el=tree#diff-cGFuZGFzL2lvL2dicS5weQ==) | `25% <0%> (-58.34%)` | :arrow_down: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/17739?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/17739?src=pr&el=footer). Last update [37860a5...ec7b086](https://codecov.io/gh/pandas-dev/pandas/pull/17739?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

any final comments?
I already stated my reservations about the need for this. 
So ideally it would be good to hear some more opinions from other devs (because if most are OK with this, it's fine for me too, I don't care about it *too* much)
I feel the same as Joris, and will defer to others on this.
As we have already capitulated long ago on caring about microperformance (https://mail.python.org/pipermail/pandas-dev/2016-March/000499.html) I don't have strong feelings. If it does not cause us a maintenance hardship (maybe it does...), and the performance is better, why not keep them? 
perf is almost exactly the same as ``.iat/.at``; they are in fact wrappers around the internal versions of ``.set_value/.get_value``. but absolutely this is an API/maintenance burden. too much surface area.
Since they are indeed rather thin wrappers around the internal set/get_value, I don't think they are actually that much of a maintenance burden. But it is true it *is* an API 'burden' (they add to the long list of available functions that users have to cope with). 
So it's a bit a trade-off. But they are still a factor of around 3x faster as `.at`. So given that I would tend to keep them (although I never use them myself).
you are completely missing the point.

They duplicate APIs. They are not tested in the least and they are very buggy. The entire point of .iat/.at is that they follow a common path and ARE tested with all dtypes. 

We are not going to keep around buggy code that has duplicate API's. This such a maintance burden. We need to actively be removing code, not preserving untested, non-documented duplicative API's.

I personally would rather have correct code.
I wouldn't say he / I are missing the point but we have a slightly different perspective =) I am +0 with removing the APIs in the interest of simplification and maintainability. 
If you read my previous post, I think you can see that I am not completely missing the point. I *agree* that it is duplicate API. But there is still a trade-off between duplicate api vs speed / breaking people's code. 
But I do not agree they are not tested. They are tested by the many indexing tests that use them under the hood.

But to the point: by further looking at it, I am certainly fine with the idea of deprecating those functions. As I would also like to clean up the API, and iat/at are I think nicer. 
But, `at`/`iat` are actually also only very small wrappers around `get_value`/`set_value`. They also do almost as little data validation as get_value/set_value (eg the bug report about PeriodIndex with get_value (https://github.com/pandas-dev/pandas/issues/15268), you have exactly the same using `at`). And I think in general this limited validation is actually the feature of those functions/methods (although the fact it cannot handle a Period on a PeriodIndex could be seen as a bug for `at`). 
And the little bit extra validation they do (the reason they are slower) is IMO not that useful. So I would like to hear if you would be OK with making at/iat a bit faster (closer to get_value/set_value).

Eg, if you look at the `at` implementation for getting, it does the following validation:

- check that it is a tuple -> otherwise raise ValueError
- on integer axis, check that key is integer -> otherwise ValueError
- on non-integer axis, check that key is not integer -> otherwise raise ValueError

But in all those case, when not doing this check, you would get a KeyError otherwise with the `get_value` that is used under the hood. But those checks make up for a big part of the reason that `at` is 3x as slow as `get_value`. 
Are those checks necessary? They provide a little bit a more informative error message (compared to just KeyError). And dropping the checks would be an API change from ValueError to KeyError. But IMO they are not really important, and I would be in favor to drop them, making `at` closer in speed to `get_value`.

Would you be with such a change?

(and to repeat from above, I am OK with the idea of removing `get_value`/`set_value`)




> Eg, if you look at the at implementation for getting, it does the following validation:

> check that it is a tuple -> otherwise raise ValueError
on integer axis, check that key is integer -> otherwise ValueError
on non-integer axis, check that key is not integer -> otherwise raise ValueError
But in all those case, when not doing this check, you would get a KeyError otherwise with the get_value that is used under the hood. But those checks make up for a big part of the reason that at is 3x as slow as get_value.
Are those checks necessary? They provide a little bit a more informative error message (compared to just KeyError). And dropping the checks would be an API change from ValueError to KeyError. But IMO they are not really important, and I would be in favor to drop them, making at closer in speed to get_value.

> Would you be with such a change?

I guess my overarching point is that we have a framework that we can/should test ``.iat/at`` more. So for sure testing is +1. We already have some scalar fast paths that least to these routines, but some of those changes are certainly possible/welcome. Obviously not averse to speeding things up.

Things done in the unified indexing context are always welcome.

didn't see any doc comments left, can you point.