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

[jbrockmendel]

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.

[jbrockmendel]

This is nearly cut/paste from the existing Block._try_cast_result implementation. The difference is this uses e.g. is_integer_dtype(left) instead of left.is_integer. The is_foo attributes map directly onto the is_foo_dtype checks with the exception of is_bool, which requires this extra object-dtype check (this is the same way Index.is_boolean works).

[codecov]

Codecov Report

Merging #22019 into master will decrease coverage by <.01%.
The diff coverage is 100%.

@@            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%)	⬇️
#single	42.36% <12.5%> (ø)	⬆️

Impacted Files	Coverage Δ
pandas/core/ops.py	97.25% <100%> (-0.16%)	⬇️
pandas/core/frame.py	97.19% <100%> (-0.01%)	⬇️
pandas/core/internals/blocks.py	93.48% <0%> (-0.37%)	⬇️
pandas/core/indexes/period.py	93.48% <0%> (-0.18%)	⬇️
pandas/core/indexes/datetimes.py	95.66% <0%> (-0.12%)	⬇️
pandas/core/indexes/base.py	96.39% <0%> (-0.11%)	⬇️
pandas/core/panel.py	97.89% <0%> (-0.02%)	⬇️
pandas/core/sparse/series.py	95.22% <0%> (-0.02%)	⬇️
pandas/core/series.py	93.74% <0%> (-0.01%)	⬇️
... and 9 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 08ecba8...5832c2b. Read the comment docs.

[jreback]

pls don't repeat code for the unique / non-unique case

[jbrockmendel]

This can be trimmed down a bit, but is based on the existing _combine_frame code. I'm anticipating that after combine_const is "fixed" most of this will move over to core.ops and we'll put some effort into being less verbose.

[jreback]

IF you need this function, it should be in panda/core/dtypes/cast.py. I say IF because this looks like lots of other functions there. please see if you can simplify. we want to remove things from internals not add them.

[jbrockmendel]

it should be in panda/core/dtypes/cast.py

Sure, though I'm hoping we can get rid of it before long.

I say IF because this looks like lots of other functions there

This is based on the existing Block._try_cast_result, which is changed to just call this function.

we want to remove things from internals not add them.

Very much so. The point of this PR (along with another branch in progress) is to remove Block.eval and BlockManager.eval. (and in doing so ensure that Series/DataFrame ops all agree)

[jbrockmendel]

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.

[jbrockmendel]

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

[jreback]

why did this change?

[jbrockmendel]

Same as below

[jreback]

I think this PR needs a note in the what's new describing this change (as its effectievly an API change)

[jreback]

this changing is not great, the point of these ops is that the dtypes won't change on the pivot

[jbrockmendel]

The change is happening because ATM the casting is done at the Block-level, while with the PR casting is done at the column-level. pd.crosstab(df.a, df.b, normalize='columns') ATM returns

b    3    4
a          
1  0.5  0.0
2  0.5  1.0

and under the PR gives:

b    3    4
a          
1  0.5  0
2  0.5  1

i.e. the 4 column is int64. Note that the original inputs df.a and df.b are both int64 to begin with. I don't know crosstab well enough to comment on the ideal output, but it's hard to call this less-correct.

[TomAugspurger]

I think the correct way to answer this is ask "was the upcast from int to float the result of coercing to a single dtype for multiple columns?" If so, then we should preserve integer dtype where we can, i.e.

For an all-integer crosstab, we preserve integer dtype:

In [27]: a = pd.Series([1, 2])

In [28]: pd.crosstab(a, a)
Out[28]:
col_0  1  2
row_0
1      1  0
2      0  1

so this change seems good (and maybe worth explicitly testing rather than astyping the output, to ensure that we don't regress)?

[jreback]

the reason these are dispatched to internals is so that the proper casting can happen. why is moving operations code here a good idea, rather than internals? we want to ove code out of frame, not in

[jbrockmendel]

why is moving operations code here a good idea, rather than internals? we want to ove code out of frame, not in

We want to move code out of internals. There are a ton of Issues about DataFrame operations that are caused by Block.eval not handling things correctly that will be fixed by using the (much more careful and well-tested) Series implementations (which BTW are the ones that correctly defer to EA and FooIndex implementations).

DataFrame ops are all over the place right now: op(DataFrame, DataFrame) and cmp(DataFrame, DataFrame) operate column-wise (dispatching to Series implementation). Other ops will either a) operate on self.values (doing expensive casting for mixed dtypes), or b) dispatch to _data.eval, which messes up on many dtypes and weird alignment behavior.

[jbrockmendel]

Note also this can be updated to be even less verbose after #22068.

@jorisvandenbossche thoughts on the dtype effects?

[jbrockmendel]

Travis error is unrelated on test_s3_roundtrip

[jbrockmendel]

CircleCI fail unrelated, hypothesis turned up an overflow

[jreback]

tiny doc comment. lgtm. push that change and ok to merge.

[jreback]

can you use :func:`crosstab` here

[jreback]

acutally rebase and wait for green to merge, just to be sure

[jbrockmendel]

Great. Once this is in, #22880 or equivalent will be the end of BlockManager.eval

[jbrockmendel]

@jreback travis fail is in s3; should I call that "green enough" or keep pushing until green-green?

[jreback]

yeah that’s fine