DEPR: deprecate get_values
State: closed

Closes https://github.com/pandas-dev/pandas/issues/19617

Reviving an old branch I had lying around. So this was more a quick experiment to see what it involves. The main "problem" is that we ourselves use `get_values` in a bunch of places (eg `values_from_object` which is massively used). 

The current situation:

- `DataFrame.get_values` simply points to `.values` so is (I think?) always a numpy ndarray.
- `Series.get_values` points to `_data.get_values()` (the SingleBlockManager), which in turn does `np.array(self._block.to_dense(), copy=False)`. 
  So the `Block.to_dense()` in general returns a view on the array, although for ExtensionBlock it does `np.array(self.values)`, with the exception for DatetimeTZBlock, which ensures a non-object array with `np.asarray(self.values, dtype=_NS_DTYPE)`, and for CategoricalBlock, it returns the Categorical.get_values which can be a DatetimeIndex.
- `Index.get_values` points to `.values`, so is a numpy ndarray with the exception of:
  - newer Index objects which are already backed by EA: IntervalIndex returns IntervalArray here (but eg DatetimeIndex with tz aware data does return a numpy array)
  - `CategoricalIndex` overrides the parent method to return the `get_values` of the underlying Categorical, which in turn mostly returns an ndarray, but an EA for interval-backed dtypes and DatetimeIndex for datetimetz aware backed categorical.

Summary: a complete mess :-)

This PR is for now just an experiment to see where it is used. Now, we should see for all those places what would be the best replacement (unless we are fine with keeping such a `_internal_get_values`, but it would be good to clean this up somewhat ..)

Labels:
name: Deprecate
description: Functionality to remove in pandas

Comments:
Hello @jorisvandenbossche! Thanks for updating this PR. We checked the lines you've touched for [PEP 8](https://www.python.org/dev/peps/pep-0008) issues, and found:



















































There are currently no PEP 8 issues detected in this Pull Request. Cheers! :beers: 

##### Comment last updated at 2019-07-01 15:20:33 UTC
(wanted to make this a draft PR, but forgot and I don't see a way to change the status once created)
# [Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/26409?src=pr&el=h1) Report
> Merging [#26409](https://codecov.io/gh/pandas-dev/pandas/pull/26409?src=pr&el=desc) into [master](https://codecov.io/gh/pandas-dev/pandas/commit/a272b60aabc2a3b4128a7365e9b2b0080bcd5121?src=pr&el=desc) will **decrease** coverage by `<.01%`.
> The diff coverage is `77.77%`.

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

```diff
@@            Coverage Diff             @@
##           master   #26409      +/-   ##
==========================================
- Coverage   91.86%   91.85%   -0.01%     
==========================================
  Files         180      180              
  Lines       50794    50727      -67     
==========================================
- Hits        46660    46594      -66     
+ Misses       4134     4133       -1
```

| Flag | Coverage Δ | |
|---|---|---|
| #multiple | `90.44% <77.77%> (-0.05%)` | :arrow_down: |
| #single | `41.13% <36.11%> (-0.85%)` | :arrow_down: |

| [Impacted Files](https://codecov.io/gh/pandas-dev/pandas/pull/26409?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pandas/core/algorithms.py](https://codecov.io/gh/pandas-dev/pandas/pull/26409/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvYWxnb3JpdGhtcy5weQ==) | `94.73% <0%> (-0.05%)` | :arrow_down: |
| [pandas/core/indexes/multi.py](https://codecov.io/gh/pandas-dev/pandas/pull/26409/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvaW5kZXhlcy9tdWx0aS5weQ==) | `95.67% <100%> (+0.81%)` | :arrow_up: |
| [pandas/util/testing.py](https://codecov.io/gh/pandas-dev/pandas/pull/26409/diff?src=pr&el=tree#diff-cGFuZGFzL3V0aWwvdGVzdGluZy5weQ==) | `90.94% <100%> (ø)` | :arrow_up: |
| [pandas/core/ops.py](https://codecov.io/gh/pandas-dev/pandas/pull/26409/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvb3BzLnB5) | `94.68% <100%> (+0.02%)` | :arrow_up: |
| [pandas/core/dtypes/concat.py](https://codecov.io/gh/pandas-dev/pandas/pull/26409/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvZHR5cGVzL2NvbmNhdC5weQ==) | `96.58% <100%> (-0.46%)` | :arrow_down: |
| [pandas/core/internals/blocks.py](https://codecov.io/gh/pandas-dev/pandas/pull/26409/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvaW50ZXJuYWxzL2Jsb2Nrcy5weQ==) | `94.39% <100%> (-0.71%)` | :arrow_down: |
| [pandas/io/formats/format.py](https://codecov.io/gh/pandas-dev/pandas/pull/26409/diff?src=pr&el=tree#diff-cGFuZGFzL2lvL2Zvcm1hdHMvZm9ybWF0LnB5) | `97.91% <100%> (-0.01%)` | :arrow_down: |
| [pandas/core/sparse/frame.py](https://codecov.io/gh/pandas-dev/pandas/pull/26409/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvc3BhcnNlL2ZyYW1lLnB5) | `95.63% <100%> (+2.06%)` | :arrow_up: |
| [pandas/core/frame.py](https://codecov.io/gh/pandas-dev/pandas/pull/26409/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvZnJhbWUucHk=) | `96.88% <100%> (-0.12%)` | :arrow_down: |
| [pandas/core/sparse/series.py](https://codecov.io/gh/pandas-dev/pandas/pull/26409/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvc3BhcnNlL3Nlcmllcy5weQ==) | `94.9% <100%> (+5.91%)` | :arrow_up: |
| ... and [77 more](https://codecov.io/gh/pandas-dev/pandas/pull/26409/diff?src=pr&el=tree-more) | |

------

[Continue to review full report at Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/26409?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/26409?src=pr&el=footer). Last update [a272b60...19093e3](https://codecov.io/gh/pandas-dev/pandas/pull/26409?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

> The main "problem" is that we ourselves use get_values in a bunch of places

Agree with putting "problem" in quotes. As you show, this is surprisingly hard to use correctly. I'd be happy to see it go away.
> I'd be happy to see it go away.

Well, that's the part I didn't do yet :-) 
I currently simply replaced all internal uses with `_internal_get_values`. And if we want to get rid of that, we need to decide case by case what to use instead. 

But the problem is that `get_values` has a quite specific behaviour that is not replicable with any of `values`, `array`, `to_numpy()`, `_values` or `_ndarray_values`. 
`get_values` ensures the output is a numpy array; it does not preserve fully the information (eg looses timezone), but also avoids object dtype if possible. So it uses datetime64[ns] for datetimetz (while `to_numpy` gives object), but does convert to object for object categoricals (while `_ndarray_values` gives the integer codes here). 

I suppose in some cases, this difference might not matter, and we can replace it with eg `to_numpy()`, or we can put this "special" logic into `com.values_from_object` (which currently simply calls `get_values()`) and for all cases where we can't replace it with one of the consistent attributes/methods, use this function.
you need to be extremely careful here as this may have perf implications
rather than actually deprecating this should just change to use .to_numpy() (or _ndarray_values) as appropriate
> rather than actually deprecating this should just change to use .to_numpy() (or _ndarray_values) as appropriate

Do those two aspects not go together? We try to change it to `to_numpy` et al, so we actually *can* deprecate (and eventually remove) it?

Note that it is actually the "just change to use `to_numpy()`" that has the performance implications (eg object dtype or not). 

As explained above, the current `get_values` has a quite specific behaviour, so we probably need to keep *some* internal function or method that keeps that behaviour, exactly to avoid possible implications.
OK, to get this into a merge-able state, I see currently two options:

I think we need to keep for now the functionality of "get_values" (which means: give back *some* numpy array, special cases behaviour depending on the dtype, see above). So we can:

- centralize the special cases in `com.values_from_object`. We have one place with the logic to decide what kind of ndarray to return for this function.

- keep the special cases on their own class (like I am doing now with the `_internal_get_values`) which is then used by `com.values_from_object`.

I think I personally like the first one better, but that has possible the complication that we need to repeat the logic in the json C code (didn't check that in detail though). And also, in general we always have this logic on the object itself (eg the existing `_values`, `_ndarary_values` attributes).

> I think I personally like the first one better, but that has possible the complication that we need to repeat the logic in the json C code (didn't check that in detail though). And also, in general we always have this logic on the object itself (eg the existing _values, _ndarary_values attributes).

slightly annoying, but you could *call* ``_values_from_object`` in the json code to unify this.
looks good, just some checks failing.
Generally a big win, especially in sparse.  Can we think of a better name than internal values?  That doesn’t do much to tell me what it’s for.
“_dense_values”?
_lossless_values?  If I’m writing an EA and need to implement it, what would this be for eg RangeArray?
EA's don't have to implement this. Currently in master, Categorical is the only one that does. 

In general, the `ExtensionBlock` returns `np.array(self.values)` that will get used in those `get_values`. We only have internally the exception for DatetimeTzBlock (to return DatetimeIndex) and CategoricalBlock. That's certainly something we should try to fix, but that's a bigger endeavor than this PR. 
> _lossless_values?

It's not fully lossless (eg for categorical you loose the categories and orderedness). 

`_internal_get_values` is certainly an awful name, but on the other hand it also is more or less what it is. It is some kind of "get_values" that is for some historical reasons needed for internal use.

I hope to at some point get rid of it. But of course, that's maybe not for the short term.


As said above (https://github.com/pandas-dev/pandas/pull/26409#issuecomment-503080971), I can also try to get rid of it entirely by concentrating the special case logic in `values_from_object`
The issue about getting a "dense" array (but preserving dtype) of a Categorical is https://github.com/pandas-dev/pandas/issues/26410
@jreback do you want to do another check, or is this good to go?

(the one failure is flaky test)
lgtm but a check is failing?
Opened also a general follow-up issue for removing the use of `_internal_get_values`: https://github.com/pandas-dev/pandas/issues/27165 (next to the specific json issue: https://github.com/pandas-dev/pandas/issues/27164)
Travis failure is the pyyaml issue, so ignoring that one.