REF: Internal / External values
State: closed

Closes #19548

Just changed Index so far. Need to

- Run an ASV
- add `Series._ndarray_values`
- Start cleaning up now redundant if conditions.

Labels:
name: Refactor
description: Internal refactoring of code
name: Clean

Comments:
# [Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/19558?src=pr&el=h1) Report
> Merging [#19558](https://codecov.io/gh/pandas-dev/pandas/pull/19558?src=pr&el=desc) into [master](https://codecov.io/gh/pandas-dev/pandas/commit/d9551c8ee2a09ccc4c39d2b661f199334162edb6?src=pr&el=desc) will **increase** coverage by `0.01%`.
> The diff coverage is `97.32%`.

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

```diff
@@            Coverage Diff             @@
##           master   #19558      +/-   ##
==========================================
+ Coverage   91.57%   91.58%   +0.01%     
==========================================
  Files         150      150              
  Lines       48817    48864      +47     
==========================================
+ Hits        44704    44752      +48     
+ Misses       4113     4112       -1
```

| Flag | Coverage Δ | |
|---|---|---|
| #multiple | `89.95% <97.32%> (+0.01%)` | :arrow_up: |
| #single | `41.75% <43.75%> (+0.02%)` | :arrow_up: |

| [Impacted Files](https://codecov.io/gh/pandas-dev/pandas/pull/19558?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pandas/io/pytables.py](https://codecov.io/gh/pandas-dev/pandas/pull/19558/diff?src=pr&el=tree#diff-cGFuZGFzL2lvL3B5dGFibGVzLnB5) | `92.45% <100%> (ø)` | :arrow_up: |
| [pandas/core/base.py](https://codecov.io/gh/pandas-dev/pandas/pull/19558/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvYmFzZS5weQ==) | `96.79% <100%> (+0.01%)` | :arrow_up: |
| [pandas/core/indexes/period.py](https://codecov.io/gh/pandas-dev/pandas/pull/19558/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvaW5kZXhlcy9wZXJpb2QucHk=) | `93.04% <100%> (+0.05%)` | :arrow_up: |
| [pandas/core/indexes/datetimes.py](https://codecov.io/gh/pandas-dev/pandas/pull/19558/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvaW5kZXhlcy9kYXRldGltZXMucHk=) | `95.32% <100%> (+0.06%)` | :arrow_up: |
| [pandas/core/indexes/base.py](https://codecov.io/gh/pandas-dev/pandas/pull/19558/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvaW5kZXhlcy9iYXNlLnB5) | `96.46% <100%> (+0.02%)` | :arrow_up: |
| [pandas/core/dtypes/cast.py](https://codecov.io/gh/pandas-dev/pandas/pull/19558/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvZHR5cGVzL2Nhc3QucHk=) | `87.98% <100%> (ø)` | :arrow_up: |
| [pandas/core/indexes/numeric.py](https://codecov.io/gh/pandas-dev/pandas/pull/19558/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvaW5kZXhlcy9udW1lcmljLnB5) | `97.24% <100%> (ø)` | :arrow_up: |
| [pandas/core/dtypes/concat.py](https://codecov.io/gh/pandas-dev/pandas/pull/19558/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvZHR5cGVzL2NvbmNhdC5weQ==) | `99.14% <100%> (ø)` | :arrow_up: |
| [pandas/core/indexes/interval.py](https://codecov.io/gh/pandas-dev/pandas/pull/19558/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvaW5kZXhlcy9pbnRlcnZhbC5weQ==) | `92.44% <100%> (+0.04%)` | :arrow_up: |
| [pandas/core/arrays/categorical.py](https://codecov.io/gh/pandas-dev/pandas/pull/19558/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvYXJyYXlzL2NhdGVnb3JpY2FsLnB5) | `94.89% <100%> (+0.01%)` | :arrow_up: |
| ... and [7 more](https://codecov.io/gh/pandas-dev/pandas/pull/19558/diff?src=pr&el=tree-more) | |

------

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

Hello @TomAugspurger! Thanks for updating the PR.

















































Cheers ! There are no PEP8 issues in this Pull Request. :beers: 

##### Comment last updated on February 13, 2018 at 11:54 Hours UTC
Still a WIP, but this should hopefully pass.

I'm going to make a pass through the library to remove things that are currently doing checks for `Series` vs. `Index`, which should clean things up.

I've also cherry picked `ExtensionArray.astype` stuff from https://github.com/pandas-dev/pandas/pull/19604, so the diff in lines in this PR is a bit smaller than it looks.

Setops are even more of a mess now. I have a biggish refactor started (with nice perf boosts), but didn't finish it today. It's tricky since the current implementation was relying on `self._inner_indexer(self._values, ...)` raising in some cases that now won't raise (period or categorical I think). I think that it's OK to accept a bit of technical debt in this case, and I'll pay it down later.
``.get_values()`` should be deprecated, if you want to implement in terms of an internal function, ok. but the external API needs fixing. This was never meant to be exposed and is useless. let's clean that up first
> external API needs fixing

What needs to be fixed? Or do just mean deprecating it?

>  let's clean that up first

That doesn't need to happen first, it can be done independently of this. I will open an issue for it.

You were faster :-) https://github.com/pandas-dev/pandas/issues/19617
This one is all green. To summarize:

1. `._values` is either an `ndarray`, `ExtensionArray`, or `Index` (for `DatetimeIndex w/ TZ)
2. `._ndarray_values` is always an `ndarray`

The changes were

- `PeriodIndex([])._values`: ordinals -> object array (soon a `PeriodArray`)
- `DatetimeIndex[tz]._values`: ndarray at UTC -> `DatetimeIndex` (soon a `DatetimeTZArray)
I think you might want to inherit the Indexes from EA before you attempt to do this PR.
Are indexes inheriting from EA the plan, or will we do composition like CategoricalIndex composes a Categorical. I think I like composition a bit more, but both have their merits

________________________________
From: Jeff Reback <notifications@github.com>
Sent: Saturday, February 10, 2018 9:46:06 AM
To: pandas-dev/pandas
Cc: Tom Augspurger; Mention
Subject: Re: [pandas-dev/pandas] [WIP]REF: Internal / External values (#19558)


I think you might want to inherit the Indexes from EA before you attempt to do this PR.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub<https://github.com/pandas-dev/pandas/pull/19558#issuecomment-364664191>, or mute the thread<https://github.com/notifications/unsubscribe-auth/ABQHIm_4ApivlYtdN1d_sv8vvnO9V82kks5tTbm-gaJpZM4R7rd_>.

Indexes are perfect candidates for EA. 
I didn't think about it in detail or looked at the code in detail, but, I think composition would make more sense. Series will not go the inheritance way but will do composition, so doing the same for Index will keep those two more in line. 
And, it is also more in line to how it currently already is for the indexes based on extension dtypes (period, categorical), so this will be a less big change.
> Are indexes inheriting from EA the plan, or will we do composition like CategoricalIndex composes a Categorical. I think I like composition a bit more, but both have their merits

+1 for composition.  

> ._values is either an ndarray, ExtensionArray, or Index (for `DatetimeIndex w/ TZ)
._ndarray_values is always an ndarray

Double-checking I understand the promise/contract here: can I think of `obj._ndarray_values` as "unwrap `obj` until we get to a non-pandas [usually numpy] object; may be lossy" and `obj._values` as "unwrap `obj` as much as we can without being lossy"?

More specifically, will it always be the case that `op(obj, other)` can wrap `op(obj._values, other)` for supported (arith, comparison, [...?]) ops?

Is `_ndarray_values` specifically `np.ndarray` or is this being left open for e.g. pyarrow in the future or geopandas/dask to get creative with?  If the latter, is there a requirement on what the returned object should look like?
> Double-checking I understand the promise/contract here: can I think of obj._ndarray_values as "unwrap obj until we get to a non-pandas [usually numpy] object; may be lossy" and obj._values as "unwrap obj as much as we can without being lossy"?

I think rather as it like: '_ndarray_values' -> the "physical" data that are internals (indexing machinery, algos) use, so for numeric data this is just itself, but for Categorical it are the integer codes, for PeriodIndex the integer ordinals. And '_values' -> the array-like that preserves all information, so for those that are not represented in numpy type system, our own array class.

> More specifically, will it always be the case that op(obj, other) can wrap op(obj._values, other) for supported (arith, comparison, [...?]) ops?

That's a further design discussion that we need to have I think (https://github.com/pandas-dev/pandas/issues/19577, I thought you opened an issue about it as well, but can't find it). Ideally that sounds like a nice design, but I am not familiar enough with all current dispatching or not dispatching between Index, Series, blocks, .. to know how possible this would be. 


Gotcha. I'm not sure either, but it'd complicate things I suspect, since we
don't have a PeriodBlock like we do for DatetimeTZ.

Regardless, this will change in the near future to be a PeriodArray.



On Sun, Feb 11, 2018 at 2:50 PM, Joris Van den Bossche <
notifications@github.com> wrote:

> *@jorisvandenbossche* commented on this pull request.
> ------------------------------
>
> In pandas/core/indexes/base.py
> <https://github.com/pandas-dev/pandas/pull/19558#discussion_r167449141>:
>
> > +        Both ``_values`` and ``_ndarray_values`` are consistent between
> +        ``Series`` and ``Index``.
> +
> +        It may differ from the public '.values' method.
> +
> +        index             | values          | _values     | _ndarray_values |
> +        ----------------- | -------------- -| ----------- | --------------- |
> +        CategoricalIndex  | Categorical     | Categorical | codes           |
> +        DatetimeIndex[tz] | ndarray[M8ns]   | DTI[tz]     | ndarray[M8ns]   |
> +
> +        For the following, the ``._values`` is currently ``ndarray[object]``,
> +        but will soon be an ``ExtensionArray``
> +
> +        index             | values          | _values      | _ndarray_values |
> +        ----------------- | --------------- | ------------ | --------------- |
> +        PeriodIndex       | ndarray[object] | ndarray[obj] | ndarray[int]    |
>
> Is it just that ._values being a PeriodIndex would be more consistent with
> ._values returning a DatetimeIndex?
>
> Yes. So regardless of what ._values currently does or doesn't return for
> Index/Series. Just from looking at your table, returning PeriodIndex
> would make more sense IMO, as this is (at this moment) the closest thing to
> an array-like that preserves the information.
> But, I don't know what implication it would have for Series ops that
> ._values would start returning a PeriodIndex instead of object array of
> Periods, so it was really just a question, not a request for change.
>
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub
> <https://github.com/pandas-dev/pandas/pull/19558#discussion_r167449141>,
> or mute the thread
> <https://github.com/notifications/unsubscribe-auth/ABQHIphuJbL5OS7V268SM_K_V_b-5Ux4ks5tT1KWgaJpZM4R7rd_>
> .
>

`._data` seems even more inconsistent

class             | output
----------------- | ------
Index             | ndarray
PeriodIndex       | ndarray[int] (ordinals)
DatetimeIndex     | DatetimeIndex (?)
DatetimeIndex[tz] | ndarray[datetime64[ns]]
CategoricalIndex  | Categorical
IntervalIndex     | None

It hopefully wouldn't be much work to convert uses of `._data` to `._values`, but I don't have any plans to do that right now. I'll open up a followup issue though. https://github.com/pandas-dev/pandas/issues/19658
Thanks for the review. I implemented your suggestions where I didn't comment in reply, and tests pass locally.
> It hopefully wouldn't be much work to convert uses of ._data to ._values, but I don't have any plans to do that right now. I'll open up a followup issue though. #19658

\+ 1, that only just got me thinking. If you create an index from an extension array (directly, or eg by doing `set_index` on a column backed by extension block with extension array), I suppose this creates an object Index (with array of scalars) right now? Might be good to add a test for that? (but that probably belongs in the other PR that implements storing an EA in Series)
> I suppose this creates an object Index (with array of scalars) right now? Might be good to add a test for that? (but that probably belongs in the other PR that implements storing an EA in Series)

Yes, and agreed (we hit this indirectly though `._value_counts`), but it'd be good to have an explicit test.
Implemented the changes dicussed in https://github.com/pandas-dev/pandas/pull/19558#discussion_r167650514: `.shape` and `.size` use `self._values`.

I implemented those on `PeriodIndex` and `DatetimeIndex`, else we would have materialized the array of objects (which is what we do on master), so we get some performance gains there I guess

master:

```python
In [6]: idx = pd.interval_range(0, 100_000)

In [8]: %timeit idx.size
308 ns ± 9.8 ns per loop (mean ± std. dev. of 7 runs, 1000000 loops each)
```

pr:

```python
In [3]: %timeit idx.size
1.05 µs ± 9.63 ns per loop (mean ± std. dev. of 7 runs, 1000000 loops each)
```
do 'master' and 'pr' need to be swapped?
Not sure what happened on that timing I posted earlier. I can't reproduce it.

```python
In [1]: import pandas as pd

In [2]: idx = pd.interval_range(0, 100_000)

In [3]: %time idx.shape
CPU times: user 177 ms, sys: 10.1 ms, total: 188 ms
Wall time: 187 ms
Out[3]: (100000,)
```

```python
In [1]: import pandas as pd

In [2]: idx = pd.interval_range(0, 100_000)

In [3]: %time idx.shape
CPU times: user 18 µs, sys: 2 µs, total: 20 µs
Wall time: 22.9 µs
Out[3]: (100000,)
```
CI is green again.
Agreed entirely about the hackiness, especially in the set-ops. I should be able to do it in parallel with my Interval ExtensionArray PR.
All green. Merging.
Great!
@TomAugspurger this broke the build. what is going on here?


actually looks like just a network failure ok. But before merge anything else all of the other builds need to work.