[FEATURE] Pivot implementation #1645
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
|
|
Codecov Report
@@ Coverage Diff @@
## master #1645 +/- ##
==========================================
+ Coverage 81.88% 81.93% +0.05%
==========================================
Files 79 79
Lines 9353 9379 +26
==========================================
+ Hits 7659 7685 +26
Misses 1694 1694
Continue to review full report at Codecov.
|
dchigarev
force-pushed
the
pivot-implementation
branch
from
641adc2 to
408dbbd
Compare
|
At |
|
Ok @dchigarev I will place a blocked tag on this PR. Feel free to remove it once #1649 is merged. |
|
@devin-petersohn I've add some fixes into But there is some mismatch from pandas in this implementation. If our specified |
|
@dchigarev pandas 1.1 (August 1) will have a new feature added in pandas-dev/pandas#30584 to keep or drop |
|
@devin-petersohn I've finished implementing
|
dchigarev
force-pushed
the
pivot-implementation
branch
from
dee328b to
a1c6c90
Compare
There was a problem hiding this comment.
I ran some timings to see how this implementation performs, and I noticed that utilization is very high (good), but overall time is a bit slow.
On 30,000 rows 3 columns, pandas takes 166 ms, this PR takes >30s. The performance is highly dependent on the number of unique values in the index column. With this case, defaulting to pandas takes 250ms.
Did you run performance on this yourself? I would not expect this performance seeing the implementation and knowing how groupby is implemented.
5 tasks
|
@devin-petersohn Yeah, the problem seems to appear with increasing amount of Reduce part in (please note if this method of measurement is incorrect) groupby_reduce @classmethod
def groupby_reduce(
cls, axis, partitions, by, map_func, reduce_func
): # pragma: no cover
from timeit import default_timer as timer
t1 = timer()
map_func = ray.put(map_func)
by_parts = np.squeeze(by)
if len(by_parts.shape) == 0:
by_parts = np.array([by_parts.item()])
print("GRP_preparations:", timer() - t1)
t1 = timer()
new_partitions = np.array(
[
[
PandasOnRayFramePartition(
func.remote(
part.oid,
by_parts[col_idx].oid if axis else by_parts[row_idx].oid,
map_func,
part.call_queue,
by_parts[col_idx].call_queue
if axis
else by_parts[row_idx].call_queue,
)
)
for col_idx, part in enumerate(partitions[row_idx])
]
for row_idx in range(len(partitions))
]
)
[y.get() for x in new_partitions for y in x]
print("GRP_map:", timer() - t1)
t1 = timer()
res = cls.map_axis_partitions(axis, new_partitions, reduce_func)
[y.get() for x in res for y in x]
print("GRP_reduce:", timer() - t1)
return resAnd I've got this result for And if you'll look at CPU usage at the reduce part, probably only one core would be used. Let's look at two corner cases:
Let's say we have
Adding this line before reduce part to trigger resplitting at rows axis, gives increase of performance in 5x times new_partitions = cls.map_axis_partitions(1, new_partitions, lambda df: df)Execution time for
It seems that groupby + apply for high dimension data is pretty hard for pandas, for example for by = pd_df["_index"]
pd_df.drop(columns=["_index", "_columns"]).groupby(by).apply(lambda df: df)So I think unsplitted data passed to the reduce function is a problem here. P.S. Also tested the previous implementation of 766cc14 still has places to optimize and requires refactoring, this uses draft |
dchigarev
force-pushed
the
pivot-implementation
branch
3 times, most recently
from
1995ae4 to
fdbaaab
Compare
|
@devin-petersohn I've refactored |
There was a problem hiding this comment.
@dchigarev It looks reasonable using unstack here, I will leave comments about unstack implementation on #1649.
dchigarev
force-pushed
the
pivot-implementation
branch
3 times, most recently
from
a523f1b to
ef58744
Compare
|
@devin-petersohn since |
dchigarev
force-pushed
the
pivot-implementation
branch
from
ef58744 to
0b64517
Compare
dchigarev
force-pushed
the
pivot-implementation
branch
from
0b64517 to
ef9ff37
Compare
Signed-off-by: Dmitry Chigarev <[email protected]>
dchigarev
force-pushed
the
pivot-implementation
branch
from
ef9ff37 to
668987a
Compare
|
@devin-petersohn since |
devin-petersohn
approved these changes
Aug 31, 2020
There was a problem hiding this comment.
The performance of this will not be very fast for this first implementation, but we will tune stack and unstack when #1975 is resolved.
aregm
pushed a commit
to aregm/modin
that referenced
this pull request
Sep 16, 2020
…ect#1645) Signed-off-by: Dmitry Chigarev <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What do these changes do?
flake8 modinblack --check modingit commit -sThat implementation uses
unstaskfunction that falls default to pandas in current master, but after #1649 will be merged it will not