[CLN] Dispatch (some) Frame ops to Series, avoiding _data.eval
State: closed

ATM there are exactly two places where `BlockManager.eval` is called: `DataFrame._combine_match_columns` and `DataFrame._combine_const`.  This replaces the usage in `_combine_match_columns` with a dispatch-to-Series implementation.  Some output dtypes get changed (see edits in `test_axis_select_reindex`, `test_pivot`), and some errors get changed from `ValueError` to `TypeError` (see `test_operators`).

The other usage of `_data.eval` will be removed separately; that turns out to be a lot more trouble because a bunch of `DataFrame` behavior is currently incorrect (see #22017).

This PR also:
- Simplifies some of the special-casing in SparseDataFrame; trying to move towards not having separate implementations for these methods
- Dispatches `_combine_match_index` to avoid calling `self.values` when doing so would require coercing to object-dtype.

Labels:
name: Refactor
description: Internal refactoring of code
name: Internals
description: Related to non-user accessible pandas implementation

Comments:
# [Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/22019?src=pr&el=h1) Report
> Merging [#22019](https://codecov.io/gh/pandas-dev/pandas/pull/22019?src=pr&el=desc) into [master](https://codecov.io/gh/pandas-dev/pandas/commit/08ecba8dab4a35ad3cad89fe02c7240674938b97?src=pr&el=desc) will **decrease** coverage by `<.01%`.
> The diff coverage is `100%`.

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

```diff
@@            Coverage Diff             @@
##           master   #22019      +/-   ##
==========================================
- Coverage   92.18%   92.18%   -0.01%     
==========================================
  Files         169      169              
  Lines       50833    50836       +3     
==========================================
- Hits        46862    46861       -1     
- Misses       3971     3975       +4
```

| Flag | Coverage Δ | |
|---|---|---|
| #multiple | `90.59% <100%> (-0.01%)` | :arrow_down: |
| #single | `42.36% <12.5%> (ø)` | :arrow_up: |

| [Impacted Files](https://codecov.io/gh/pandas-dev/pandas/pull/22019?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pandas/core/ops.py](https://codecov.io/gh/pandas-dev/pandas/pull/22019/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvb3BzLnB5) | `97.25% <100%> (-0.16%)` | :arrow_down: |
| [pandas/core/frame.py](https://codecov.io/gh/pandas-dev/pandas/pull/22019/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvZnJhbWUucHk=) | `97.19% <100%> (-0.01%)` | :arrow_down: |
| [pandas/core/internals/blocks.py](https://codecov.io/gh/pandas-dev/pandas/pull/22019/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvaW50ZXJuYWxzL2Jsb2Nrcy5weQ==) | `93.48% <0%> (-0.37%)` | :arrow_down: |
| [pandas/core/indexes/period.py](https://codecov.io/gh/pandas-dev/pandas/pull/22019/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvaW5kZXhlcy9wZXJpb2QucHk=) | `93.48% <0%> (-0.18%)` | :arrow_down: |
| [pandas/core/indexes/datetimes.py](https://codecov.io/gh/pandas-dev/pandas/pull/22019/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvaW5kZXhlcy9kYXRldGltZXMucHk=) | `95.66% <0%> (-0.12%)` | :arrow_down: |
| [pandas/core/indexes/base.py](https://codecov.io/gh/pandas-dev/pandas/pull/22019/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvaW5kZXhlcy9iYXNlLnB5) | `96.39% <0%> (-0.11%)` | :arrow_down: |
| [pandas/core/panel.py](https://codecov.io/gh/pandas-dev/pandas/pull/22019/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvcGFuZWwucHk=) | `97.89% <0%> (-0.02%)` | :arrow_down: |
| [pandas/core/sparse/series.py](https://codecov.io/gh/pandas-dev/pandas/pull/22019/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvc3BhcnNlL3Nlcmllcy5weQ==) | `95.22% <0%> (-0.02%)` | :arrow_down: |
| [pandas/core/series.py](https://codecov.io/gh/pandas-dev/pandas/pull/22019/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvc2VyaWVzLnB5) | `93.74% <0%> (-0.01%)` | :arrow_down: |
| ... and [9 more](https://codecov.io/gh/pandas-dev/pandas/pull/22019/diff?src=pr&el=tree-more) | |

------

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

Changes in returns dtypes (see tests) merit discussion since they represent an API change.  Effectively some ops that currently return e.g. all-float will now return mixed float/int dtypes.
@jreback just reverted a large chunk of non-central changes, made the remaining change in core.frame much less verbose.

This (along with the in-progress branch that gets rid of the other use of `BlockManager.eval`+`Block.eval`) will be a huge win.  We clear out a bunch of internals _and_ make Series/DataFrame ops consistent.  A _bunch_ of issues will be closed as a consequence of the latter.

This changes some return types, so will need an API Changes note if it moves forward.
Note also this can be updated to be even less verbose after #22068.

@jorisvandenbossche thoughts on the dtype effects?
@jorisvandenbossche when convenient can you weigh in on the dtype issue described above?
@jreback thoughts on @TomAugspurger’s comments?  My main goals here are to a) simplify DataFrame ops and b) get rid of Block/BlockManager methods.
in the crosstab example what is the input look like?
> in the crosstab example what is the input look like?

On L1569 in tests/reshape/test_pivot.py we call `pd.crosstab(df.a, df.b, normalize='columns')`.  In that, `df` is given by:

```
>>> df = pd.DataFrame({'a': [1, 2, 2, 2, 2], 'b': [3, 3, 4, 4, 4], 'c': [1, 1, np.nan, 1, 1]})
>>> df
   a  b    c
0  1  3  1.0
1  2  3  1.0
2  2  4  NaN
3  2  4  1.0
4  2  4  1.0
```

> i have a couple of unresolved comments above

Can you be more specific?  AFAICT all your comments have been responded to; not sure which ones you consider unresolved.

> what is perf differences ?

Some %timeit trials:

```
df = pd.DataFrame({'a': np.arange(1000), 'b': np.arange(1000)*2})
%timeit out = pd.crosstab(df.a, df.b, normalize='columns')
--> 481 ms on PR, 289ms on master
--> presumably overhead from DataFrame construction, possibly consolidation

%timeit out = df + df.iloc[0]
--> 974 µs on PR, 398 on master
--> makes sense since the master version is taking advantage of contiguous blocks
```

So this does entail a perf hit.  I expect that to be ameliorated by #22284, but the fact remains that this is AFAIK the main upside of the BlockManager in the first place.

I maintain this performance hit is less important than the correctness-improvement this PR brings.  For example (not an extant test, but I'll add it and others, ideally after #22350):

```
now = pd.Timestamp.now()
df = pd.DataFrame([[now, now], [now, now]])
ser = pd.Series([now, now])
>>> df - ser
```

Under this PR we correctly get:
```
       0      1
0 0 days 0 days
1 0 days 0 days
```

In master we get dtypes wrong:
```
           0          1
0 1970-01-01 1970-01-01
1 1970-01-01 1970-01-01
```
Hello @jbrockmendel! Thanks for updating the PR.

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

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

 - There are no PEP8 issues in the file [`pandas/tests/frame/test_axis_select_reindex.py`](https://github.com/jbrockmendel/pandas/blob/ff96c0deb5507f1b89fdb2d36ce9d913debf71ed/pandas/tests/frame/test_axis_select_reindex.py) !

 - There are no PEP8 issues in the file [`pandas/tests/reshape/test_pivot.py`](https://github.com/jbrockmendel/pandas/blob/ff96c0deb5507f1b89fdb2d36ce9d913debf71ed/pandas/tests/reshape/test_pivot.py) !

 - There are no PEP8 issues in the file [`pandas/tests/series/test_operators.py`](https://github.com/jbrockmendel/pandas/blob/ff96c0deb5507f1b89fdb2d36ce9d913debf71ed/pandas/tests/series/test_operators.py) !


PendingDeprecationWarning in xlrd
that must be a new warning from xlrd?

can u pin the last version and create an issue to fix the warning?
Rebase notwithstanding, thoughts on this?  If this goes through then there is only one use of BlockManger.eval left, and we can focus on fixing the last inconsistencies between Series vs DataFrame ops.
Travis error is unrelated on test_s3_roundtrip
CircleCI fail unrelated, hypothesis turned up an overflow
acutally rebase and wait for green to merge, just to be sure
Great.  Once this is in, #22880 or equivalent will be the end of BlockManager.eval
@jreback travis fail is in s3; should I call that "green enough" or keep pushing until green-green?
yeah that’s fine