REF: dont set ndarray.data in libreduction
State: closed

cc @WillAyd I'm still seeing 88 test failures locally and could use a fresh pair of eyes on this.  Any thoughts?

Labels:
name: Internals
description: Related to non-user accessible pandas implementation

Comments:
Tentatively looks like the issue is that this patches `_index_data` without updating `_data`
If it helps I've noticed that changing this line:

https://github.com/pandas-dev/pandas/blob/42fd7e7d9a2c115af9a52f7f896d48b75a271efe/pandas/_libs/reduction.pyx#L357

To `chunk = slider.frame[starts[i]:ends[i]].copy()` takes 88 failures down to about 20. Not a real solution as we don't want the copy, but I think this new design isn't fully compatible with some of the `dummy` work that the sliders do, so might need to patch together

Seems heading in the right direction though
closing to clear the queue
Of the four remaining failures one of them is a result of groupby.tshift which maybe we will deprecate in #34452
@WillAyd is there cause for optimism here?  A solution here might have a bearing on one of the test failures in #35417
Well the optimism is that we are down to a handful of failures :-)

I think though that this is uncovering deep seeded issues in other parts of the code base. For instance, this is causing the test that https://github.com/pydata/pandas/issues/33439 added to fail which right now I've traced back to a potential bug somewhere in our hashing ocde, but I think it's only happen stance that that test works in the first place.

Not sure if the other 4 failures are related to hashing as well but will hopefully figure out soon
I've noticed that as an apply slides along various groups that this line of code looks problematic with the current changes:

https://github.com/pandas-dev/pandas/blob/3b1d4f1eef46a45b7d32694bb263a2e40a933e74/pandas/_libs/index.pyx#L231

The problem is that the call to `_get_index_values` hits a cached property that with this doesn't seem to be updated, and only ever references the index of the first group. So if you tried to split a DataFrame with 3 elements into [0], and [1, 2] groupings, a hashtable only ever gets built for the [0] group, the next iteration throws a KeyError when trying to figure out the location of [1, 2] and a whole slew of exception handling routes that off to space from there

@jbrockmendel any idea how the caching should be handled here? In the Python space the above call traces back to here:

https://github.com/pandas-dev/pandas/blob/3b1d4f1eef46a45b7d32694bb263a2e40a933e74/pandas/core/indexes/base.py#L556


Here's the MRE I'm using to tackle the above comment.

```python
df = pd.DataFrame({"A": ["S", "W", "W"], "B": [1.0, 1.0, 2.0]}) 
res = df.groupby("A").agg({"B": lambda x: x.get(x.index[-1])})
```

On master this yields 

```python
     B
A     
S  1.0
W  2.0
```

But with this PR yields

```python
     B
A     
S  1.0
W  NaN
```

Because of the aforementioned KeyError when trying to locate elements by index in the second grouping
> The problem is that the call to _get_index_values hits a cached property

I'm not sure I follow.  In index.pyx this is a call to `self.vgetter()`, which I think is supposed to return `_index_data`, which gets patched within libreduction.  Am I misunderstanding?

`cache_readonly`s on the Index object definitely seem like a footgun.  In the general case, the only way I can think of to avoid this is to create a fresh Index object, but avoiding that overhead is basically the whole point of the shenanigans in libreduction.

Where is the call to `_index_data`? That would seem related but I wasn't seeing that; could be the missing link
> Where is the call to _index_data? That would seem related but I wasn't seeing that; could be the missing link

I expected to see it in Index._engine, but apparently not
/azp run
<samp>
Azure Pipelines successfully started running 1 pipeline(s).<br>

</samp>
Just pushed after applying a patch from #35417.  Locally that gets me to 3 test failures (which now that i reread the thread may not actually be an improvement)
Poking at this, some of the failures occur because we pass Series that have EA-backed indexes in to libreduction.  This then gets swallowed when we catch ValueError in addition to KeyError in `SeriesGroupBy.aggregate`.  Trouble is: if we avoid the libreduction path when we have these indexes, a bunch of different tests fail.  So still a ways to go.
OK i've got the failures down to one test, and the edits necessary to get there are orthogonal to the rest of this PR.  Asking for extra eyes (cc @WillAyd) on the last case.

The necessary edit is in SeriesGroupBy.aggregate, before
```
                try:
                    return self._python_agg_general(func, *args, **kwargs)
                except (ValueError, KeyError):
                    # TODO: KeyError is raised in _python_agg_general,
                    #  see see test_groupby.test_basic
                    result = self._aggregate_named(func, *args, **kwargs)
```

add a check for 

```
if isinstance(self._selected_obj.index, (DatetimeIndex, TimedeltaIndex, PeriodIndex):
    result = self._aggregate_named(func, *args, **kwargs)
else:
   [ try _python_agg_general ]
```

With that edit alone you'll get about 10 test failures, 9 of which i can fix.  The remaining one is `tests.resample.test_resampler_grouper.test_apply`:

```
from pandas.tests.resample.test_resampler_grouper import *

g = test_frame.groupby("A")
r = g.resample("2s")
    
expected = g.resample("2s").sum()
    
def f(x):
    return x.resample("2s").sum()
    
result = r.apply(f)  # <-- is now incorrect
```

`result` here _should_ have a 2-level MultiIndex with the second level being a DatetimeIndex.  Instead, it now has 3 levels and the 3rd is a duplicate of the second.

AFAICT whats happening is:
1) we go through _aggregate_named instead of _python_agg_general
2) _aggregate_named raises ValueError("Must produce aggregated value")
3) this gets caught inside `r._groupby_and_aggregate` which falls back to using `grouped.apply`, which is doing something resulting in a duplicate level
When rebased on top of #35928, I'm down to just the one test failure in tests.groupby.test_allowlist `test_groupby_selection_with_methods` where it tentatively looks like that test is itself wrong
rebased on top of #35928 and #35937
Updated to implement NDFrame._can_use_libreduction to de-duplicate a bunch of similar (and inconsistently strict) checks done elsewhere.  (Note an inline-comment I'll make about a place where this PR retains a different, inconsistent check)

This fixes an xfailed tests.groupby.test_apply.test_apply_with_timezones_aware


Closing.

This doesn't do quite what I thought it did, will need a new approach, xref #36459.
This thing is super thorny...nice effort in any case here. We will figure it out one of these days