REF: use asi8 instead of _data in PeriodArray
State: closed

@TomAugspurger @jorisvandenbossche discussed on the call a couple weeks ago.  Reference `asi8` instead of `_data` or `_ndarray_values` is the most-explicit option available.

I would also be in favor of getting rid of `_data` directly and setting `asi8` in `__init__`, but holding off on that pending consensus.

Also re-implemented some simplifications that got lost in rebasing during the PeriodArray marathon.

Labels:
name: Period
description: Period data type
name: Clean

Comments:
Hello @jbrockmendel! Thanks for submitting the PR.

 - There are no PEP8 issues in the file [`pandas/core/arrays/datetimelike.py`](https://github.com/jbrockmendel/pandas/blob/f5b4da0b2d5f9b189aa2d2ff9d3c89e1a9cfd891/pandas/core/arrays/datetimelike.py) !

 - There are no PEP8 issues in the file [`pandas/core/arrays/period.py`](https://github.com/jbrockmendel/pandas/blob/f5b4da0b2d5f9b189aa2d2ff9d3c89e1a9cfd891/pandas/core/arrays/period.py) !

 - There are no PEP8 issues in the file [`pandas/core/indexes/period.py`](https://github.com/jbrockmendel/pandas/blob/f5b4da0b2d5f9b189aa2d2ff9d3c89e1a9cfd891/pandas/core/indexes/period.py) !


# [Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/23416?src=pr&el=h1) Report
> Merging [#23416](https://codecov.io/gh/pandas-dev/pandas/pull/23416?src=pr&el=desc) into [master](https://codecov.io/gh/pandas-dev/pandas/commit/de39bfc5e5c6483cb2669773fa10ddc2e32ca111?src=pr&el=desc) will **not change** coverage.
> The diff coverage is `92.3%`.

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

```diff
@@           Coverage Diff           @@
##           master   #23416   +/-   ##
=======================================
  Coverage   92.21%   92.21%           
=======================================
  Files         161      161           
  Lines       51173    51173           
=======================================
  Hits        47190    47190           
  Misses       3983     3983
```

| Flag | Coverage Δ | |
|---|---|---|
| #multiple | `90.65% <92.3%> (ø)` | :arrow_up: |
| #single | `42.23% <15.38%> (ø)` | :arrow_up: |

| [Impacted Files](https://codecov.io/gh/pandas-dev/pandas/pull/23416?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pandas/core/arrays/period.py](https://codecov.io/gh/pandas-dev/pandas/pull/23416/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvYXJyYXlzL3BlcmlvZC5weQ==) | `97.48% <92.3%> (ø)` | :arrow_up: |

------

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

What I remember from the call is that we said that `asi8` would be strange as the 'base' data attribute as we could not share that then with DatetimeArray (because for DatetimeArray that name would be wrong). Therefore I am slightly -1 on this.

For example, the `nbytes` property can be moved to a base / mixin class for DatetimeArray and PeriodArray, as both can have the same implementation (and the same will be true for some other properties/method)
I actually like _data as it very consistent across all array-like things. pls split out other changes to another PR if you would.
> What I remember from the call is that we said that `asi8` would be strange as the 'base' data attribute as we could not share that then with DatetimeArray (because for DatetimeArray that name would be wrong)

The audio issues on the call must have been much worse than we realized, since I don't recall that at all.

It wouldn't make sense to use asi8 as the name used in `DatetimeArray.__init__`, but (recall we discussed the fact that) nearly all of the ops in all three datetimelike array classes operate on the i8 values.

> For example, the nbytes property can be moved to a base / mixin class for DatetimeArray and PeriodArray, as both can have the same implementation (and the same will be true for some other properties/method)

This would, in fact, also work fine returning `self.asi8.nbytes` in such a base/mixin class.

> I actually like _data as it very consistent across all array-like things. pls split out other changes to another PR if you would.

I can split out the other stuff, but am not ready to give up on the central point here:

The ongoing trouble with values/_values/_ndarray_values/_data/... is driven by ambiguity.  When I see one of these used, it isn't immediately obvious what it means.  This PR changes usages of the maximally-ambiguous name to the maximally-obvious name.
> It wouldn't make sense to use asi8 as the name used in DatetimeArray.__init__, 

But I think it would still make sense to use the same attribute name for all datetimelike arrays, and in basic functionality (like `__len__`) it would be the most clear IMO to simply use the attribute set in the init.

I am fine with another name as `_data`, *if* there is a good alternative. `asi8` is not common for all datetime-like arrays, and for `_ndarray_values` I gave some reasoning here why that is not a good choice IMO: https://github.com/pandas-dev/pandas/pull/22862#issuecomment-431795092. 
We can still do something like `_np_data`, or other variations, if we really want.

> but (recall we discussed the fact that) nearly all of the ops in all three datetimelike array classes operate on the i8 values.

Yes, and for the ops in the PerioArray code, it uses `asi8` and not `_data`, which is fine I think.

> This would, in fact, also work fine returning self.asi8.nbytes in such a base/mixin class.

That would work, but I don't think it would be nicer or clearer design than using the 'base' data attribute.


As you mentioned in the linked comment, `_ndarray_values` is already overloaded; my point is that `_data` is equally overloaded.  Three options come to mind:

1) Accept that we are not likely to find a good name for an attribute to set in `__init__` that works for all our EA subclasses.  Instead, try to use informative names on a case-by-case basis.  One upside to this is it avoids creating "shadow interface" where we get used to EA classes having certain behavior despite not actually requiring it.

2) Standardize on a name like `_initvals` or `_attrvals` that conveys no information other than "this is set in `__init__` and is a plain attribute-lookup".  This has the advantage of being very clear.  (Also relevant: there has also been discussion elsewhere of wanting a name that guarantees fast/no-copy access.)

3) This PR doesn't try to change the name set in `__init__`, so that part of the issue could be kicked down the road.

What this PR _does_ do is replace usages of a highly-overloaded name with usages of an entirely-clear name.  If later we need to tinker with it in order to share code with other Datetimelike EAs, I'm fine with that.
> Standardize on a name like _initvals or _attrvals that conveys no information other than "this is set in __init__ and is a plain attribute-lookup". This has the advantage of being very clear. (Also relevant: there has also been discussion elsewhere of wanting a name that guarantees fast/no-copy access.)

I am not sure how this is different from ``_data``.

Not a priority, closing.