DEPR: deprecate get_values #26409

jorisvandenbossche · 2019-05-15T10:25:55Z

Closes #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 ..)

jorisvandenbossche · 2019-05-15T10:28:56Z

(wanted to make this a draft PR, but forgot and I don't see a way to change the status once created)

TomAugspurger · 2019-05-15T15:38:33Z

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.

jorisvandenbossche · 2019-05-15T19:13:55Z

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.

jreback · 2019-05-15T19:22:46Z

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

jorisvandenbossche · 2019-05-15T19:33:28Z

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.

TomAugspurger · 2019-06-17T20:19:06Z

pandas/core/arrays/categorical.py

@@ -1505,6 +1505,9 @@ def get_values(self):
            A numpy array of the same dtype as categorical.categories.dtype or
            Index if datetime / periods.
        """
+        raise Exception("USING GET_VALUES")


Should I be concerned that CI is passing with this here? :)

Perhaps we need a test ensuring that Categorical.get_values() raises a warning.

Should I be concerned that CI is passing with this here? :)

No, that's a good sign :)
As that was my way to find all the places where it was being called, to fix those (although it is not a perfect way, as in certain places the error could also be catched)

But for sure still need to convert this into a warning and add tests that checks that.

jorisvandenbossche · 2019-06-18T12:24:57Z

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

jreback · 2019-06-18T12:41:52Z

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.

pandas/core/indexes/category.py

pandas/core/indexes/base.py

pandas/core/indexes/category.py

jreback · 2019-06-28T19:42:23Z

pandas/core/internals/blocks.py

@@ -177,6 +177,9 @@ def get_values(self, dtype=None):
            return self.values.astype(object)
        return self.values

+    def get_block_values(self, dtype=None):


we really need this?

is this still needed?

Well, the JSON C code does handle blocks (if I remove the get_block_values part in objToJSON.C that I introduced in this PR, a couple tests fail).
I could name this here the same as for Series/INdex (i.e. _internal_get_values), but I prefer a distinct name to make it clear that the json code is handling blocks and not series/index (and that also makes it clear that all other places where _internal_get_values is used is not handling blocks). That will also make it easier to isolate and try to remove the block handling in the json C code.

Opened #27164 for follow-up on this JSON issue

jreback · 2019-06-28T19:43:06Z

pandas/tests/indexes/period/test_period.py

@@ -162,15 +162,15 @@ def test_values(self):

        exp = np.array([], dtype=np.object)
        tm.assert_numpy_array_equal(idx.values, exp)
-        tm.assert_numpy_array_equal(idx.get_values(), exp)
+        #tm.assert_numpy_array_equal(idx.get_values(), exp)


remove and/or use filterwarnings on the test

lol, this is what's failing :_.>

jreback · 2019-06-28T23:35:20Z

looks good, just some checks failing.

jbrockmendel · 2019-06-29T00:42:18Z

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.

jreback · 2019-06-29T02:19:52Z

“_dense_values”?

jbrockmendel · 2019-06-29T12:33:17Z

_lossless_values? If I’m writing an EA and need to implement it, what would this be for eg RangeArray?

jorisvandenbossche · 2019-06-29T14:51:17Z

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.

jorisvandenbossche · 2019-06-29T15:03:23Z

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

jorisvandenbossche · 2019-06-29T15:08:48Z

As said above (#26409 (comment)), I can also try to get rid of it entirely by concentrating the special case logic in values_from_object

jorisvandenbossche · 2019-06-29T15:20:57Z

The issue about getting a "dense" array (but preserving dtype) of a Categorical is #26410

jorisvandenbossche · 2019-06-30T21:54:48Z

@jreback do you want to do another check, or is this good to go?

(the one failure is flaky test)

jreback · 2019-06-30T22:16:00Z

lgtm but a check is failing?

jreback

lgtm, question. can you create a followup issue to actually see what we can fix of the remaining uses (unless you already did).

jreback · 2019-07-01T11:59:19Z

pandas/core/internals/blocks.py

@@ -177,6 +177,9 @@ def get_values(self, dtype=None):
            return self.values.astype(object)
        return self.values

+    def get_block_values(self, dtype=None):


is this still needed?

jorisvandenbossche · 2019-07-01T14:57:22Z

Opened also a general follow-up issue for removing the use of _internal_get_values: #27165 (next to the specific json issue: #27164)

jorisvandenbossche · 2019-07-01T15:57:32Z

Travis failure is the pyyaml issue, so ignoring that one.

jorisvandenbossche added 3 commits January 4, 2019 16:57

DEPR: Series.get_values

11c1f07

Merge remote-tracking branch 'upstream/master' into depr-get-values

590b5f5

experiment

61c3275

This comment has been minimized.

Sign in to view

jorisvandenbossche mentioned this pull request May 15, 2019

API: getting the "densified" values of a Categorical (preserving categories dtype) ? #26410

Open

more experiment

f96b352

This comment has been minimized.

Sign in to view

pandas-dev deleted a comment from codecov bot May 15, 2019

jorisvandenbossche added 3 commits May 15, 2019 13:31

more experiment

4931b9c

skip sparse get_values test

10cce45

fix json

10f92b1

fix sparse

b32d87b

jreback added the Deprecate Functionality to remove in pandas label May 16, 2019

jorisvandenbossche mentioned this pull request Jun 12, 2019

Series.round behavior with ExtensionArrays #26730

Closed

Merge remote-tracking branch 'upstream/master' into depr-get-values

fc87513

TomAugspurger reviewed Jun 17, 2019

View reviewed changes

jorisvandenbossche added 3 commits June 28, 2019 11:45

Merge remote-tracking branch 'upstream/master' into depr-get-values

5f1bca4

add warnings

19093e3

add deprecation notes

ec370af

jreback requested changes Jun 28, 2019

View reviewed changes

jreback added this to the 0.25.0 milestone Jun 28, 2019

jorisvandenbossche added 2 commits June 28, 2019 14:49

fix typo

1a7f644

clean: remove CategoricalIndex.get_values (inherit from base)

d6a9542

fix style issues

ae22440

tests

1a32d54

jreback requested changes Jul 1, 2019

View reviewed changes

jorisvandenbossche mentioned this pull request Jul 1, 2019

INT: the json C code should not deal with blocks #27164

Closed

add whatsnew note

990c72c

jorisvandenbossche mentioned this pull request Jul 1, 2019

CLN/INT: remove usage of _internal_get_values #27165

Closed

Merge remote-tracking branch 'upstream/master' into depr-get-values

064aeb4

jorisvandenbossche merged commit 46adc5b into pandas-dev:master Jul 1, 2019

jorisvandenbossche deleted the depr-get-values branch July 1, 2019 15:57

jsexauer mentioned this pull request Nov 28, 2019

DEPR: Clean up list of deprecations from prior versions #6581

Closed

1 task

jreback mentioned this pull request Dec 4, 2019

DEPR: deprecations log for removed issues #13777

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DEPR: deprecate get_values #26409

DEPR: deprecate get_values #26409

jorisvandenbossche commented May 15, 2019

This comment has been minimized.

jorisvandenbossche commented May 15, 2019

This comment has been minimized.

TomAugspurger commented May 15, 2019

jorisvandenbossche commented May 15, 2019

jreback commented May 15, 2019

jorisvandenbossche commented May 15, 2019

TomAugspurger Jun 17, 2019

jorisvandenbossche Jun 17, 2019

jorisvandenbossche commented Jun 18, 2019

jreback commented Jun 18, 2019

jreback Jun 28, 2019

jreback Jul 1, 2019

jorisvandenbossche Jul 1, 2019

jorisvandenbossche Jul 1, 2019

jreback Jun 28, 2019

jreback Jun 28, 2019

jreback commented Jun 28, 2019

jbrockmendel commented Jun 29, 2019

jreback commented Jun 29, 2019

jbrockmendel commented Jun 29, 2019

jorisvandenbossche commented Jun 29, 2019

jorisvandenbossche commented Jun 29, 2019

jorisvandenbossche commented Jun 29, 2019

jorisvandenbossche commented Jun 29, 2019

jorisvandenbossche commented Jun 30, 2019 •

edited

Loading

jreback commented Jun 30, 2019

jreback left a comment

jreback Jul 1, 2019

jorisvandenbossche commented Jul 1, 2019

jorisvandenbossche commented Jul 1, 2019

DEPR: deprecate get_values #26409

DEPR: deprecate get_values #26409

Conversation

jorisvandenbossche commented May 15, 2019

This comment has been minimized.

jorisvandenbossche commented May 15, 2019

This comment has been minimized.

TomAugspurger commented May 15, 2019

jorisvandenbossche commented May 15, 2019

jreback commented May 15, 2019

jorisvandenbossche commented May 15, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jorisvandenbossche commented Jun 18, 2019

jreback commented Jun 18, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jreback commented Jun 28, 2019

jbrockmendel commented Jun 29, 2019

jreback commented Jun 29, 2019

jbrockmendel commented Jun 29, 2019

jorisvandenbossche commented Jun 29, 2019

jorisvandenbossche commented Jun 29, 2019

jorisvandenbossche commented Jun 29, 2019

jorisvandenbossche commented Jun 29, 2019

jorisvandenbossche commented Jun 30, 2019 • edited Loading

jreback commented Jun 30, 2019

jreback left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jorisvandenbossche commented Jul 1, 2019

jorisvandenbossche commented Jul 1, 2019

jorisvandenbossche commented Jun 30, 2019 •

edited

Loading