FIX-#6632: Return Series instead of Dataframe for groupby.apply in case of experimental groupby

[Garra1980]

What do these changes do?

• first commit message and PR title follow format outlined here

  NOTE: If you edit the PR title to match this format, you need to add another commit (even if it's empty) or amend your last commit for the CI job that checks the PR title to pick up the new PR title.

• passes flake8 modin/ asv_bench/benchmarks scripts/doc_checker.py
• passes black --check modin/ asv_bench/benchmarks scripts/doc_checker.py
• signed commit with git commit -s
• Resolves BUG: groupby and apply function produced unexpected column name __reduced__ #6632
• tests added and passing
• module layout described at docs/development/architecture.rst is up-to-date

[anmyachev]

LGTM!

[anmyachev]

@dchigarev any comments?

[dchigarev]

@dchigarev any comments?

Well, the only thing to notice is that it now always triggers the computation after groupby (because of the .columns access).

Previously we had a trend of getting rid of such explicit materialization, and now we're adding them back again. The changes are introduced to the general groupby method ._wrap_aggregation(), meaning that all the aggregations will be affected by the changes, not only .apply() calls for which we have to infer a new return type.

If we decide to keep the fix, maybe we should at least move it to the .apply() method, so the other methods won't be affected?

script to measure

import modin.pandas as pd
from asv_bench.benchmarks.utils.common import execute
import modin.config as cfg
import numpy as np
from timeit import default_timer as timer

# initialize workers
pd.DataFrame([cfg.NPartitions.get() * cfg.MinPartitionSize.get()]).to_numpy()

df = pd.DataFrame({"by_col": np.tile(np.arange(10_000), 100), "a": np.arange(1_000_000), "b": np.arange(1_000_000), "c": np.arange(1_000_000)})
execute(df) # trigger import

results = []

t1 = timer()
for _ in range(10):
    results.append(df.groupby("by_col").sum())
[execute(df) for df in results]
print(timer() - t1)

t1 = timer()
res = df.groupby("by_col").sum()
print(timer() - t1)

[anmyachev]

@dchigarev any comments?

Well, the only thing to notice is that it now always triggers the computation after groupby (because of the .columns access).

Previously we had a trend of getting rid of such explicit materialization, and now we're adding them back again. The changes are introduced to the general groupby method ._wrap_aggregation(), meaning that all the aggregations will be affected by the changes, not only .apply() calls for which we have to infer a new return type.

If we decide to keep the fix, maybe we should at least move it to the .apply() method, so the other methods won't be affected?

script to measure

import modin.pandas as pd
from asv_bench.benchmarks.utils.common import execute
import modin.config as cfg
import numpy as np
from timeit import default_timer as timer

# initialize workers
pd.DataFrame([cfg.NPartitions.get() * cfg.MinPartitionSize.get()]).to_numpy()

df = pd.DataFrame({"by_col": np.tile(np.arange(10_000), 100), "a": np.arange(1_000_000), "b": np.arange(1_000_000), "c": np.arange(1_000_000)})
execute(df) # trigger import

results = []

t1 = timer()
for _ in range(10):
    results.append(df.groupby("by_col").sum())
[execute(df) for df in results]
print(timer() - t1)

t1 = timer()
res = df.groupby("by_col").sum()
print(timer() - t1)

@dchigarev Good catch!

I was surprised that even df.groupby("by_col").sum() does not calculate the columns in advance. It doesn't look that difficult.

If we decide to keep the fix, maybe we should at least move it to the .apply() method, so the other methods won't be affected?

At least yes, you are right.

Missed materialization

[Garra1980]

If we decide to keep the fix, maybe we should at least move it to the .apply() method, so the other methods won't be affected?

you mean here - https://github.com/modin-project/modin/blob/master/modin/pandas/groupby.py#L657?

[dchigarev]

you mean here - https://github.com/modin-project/modin/blob/master/modin/pandas/groupby.py#L657?

yes

[Garra1980]

you mean here - https://github.com/modin-project/modin/blob/master/modin/pandas/groupby.py#L657?

yes

Okay, pls look at another attempt