FIX-#1294: fixed 'value_counts' implementation #2730
Merged
Conversation
This file contains hidden or 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 #2730 +/- ##
==========================================
+ Coverage 85.33% 88.77% +3.44%
==========================================
Files 150 150
Lines 15955 15925 -30
==========================================
+ Hits 13615 14138 +523
+ Misses 2340 1787 -553
Continue to review full report at Codecov.
|
|
|
|
dchigarev
force-pushed
the
issue_1294
branch
7 times, most recently
from
c77b537 to
f45ede8
Compare
devin-petersohn
requested changes
Mar 2, 2021
Collaborator
devin-petersohn
left a comment
devin-petersohn
left a comment
There was a problem hiding this comment.
Thanks @dchigarev! Let me know if this makes sense
Collaborator
Author
|
@devin-petersohn implementation of Implementation of def value_counts(self, ...):
counted_values = self.groupby( # 1-st backend-API call: MapReduce + Broadcast
by=subset or self.columns, sort=not sort, dropna=dropna
).size()
if sort:
counted_values.sort_values( # 2-nd backend-API call: FullAxisFn
ascending=ascending
)
if normalize:
counted_values = ( # 3-th and 4-th API calls:
counted_values / counted_values.sum() # sum: Map
) # divide: Map + Broadcast
return counted_valuesAs we can see, there are 4 calls to the backend-API now, some of them trigger the creation of FullAxisPartitions, which means concatenate all partitions, and then split it again (expensive operation). In an ideal world, backend-API calls shouldn't be expensive, however all those things with repartitioning and broadcasting make them so. Implementations at the frontend look clean and helps to unload query compiler from unnecessary methods, but it also has the disadvantage of many backend-API calls. What is your opinion about this? |
Collaborator
|
I think it is at most 4, but the common case of the def value_counts(self, ...):
counted_values = self.groupby( # 1-st backend-API call: MapReduce + Broadcast
by=subset or self.columns, sort=sort and not ascending, dropna=dropna
).size()
if ascending:
counted_values = counted_values.sort_values( # 2-nd backend-API call: FullAxisFn
ascending=ascending
)
if normalize:
counted_values = ( # 3-th and 4-th API calls:
counted_values / counted_values.sum() # sum: Map
) # divide: Map + Broadcast
return counted_valuesLong term, this communication overhead will be resolved by the query optimizer. |
Collaborator
Author
It can't, since >>> sr = pandas.Series([1, 2, 2, 3])
>>> sr.value_counts(sort=True)
2 2
1 1
3 1
dtype: int64
>>> sr.groupby(sr, sort=True).size()
1 1
2 2
3 1In a long term perspective, it's good to have this implementation at the front, but just mentioning that in comparison with 0.8.3 we will have a noticeable performance regression in |
Collaborator
|
@dchigarev I see, yes. So Do you have performance numbers if |
Collaborator
Author
|
@devin-petersohn here is the performance with implementation at the API level data |
dchigarev
force-pushed
the
issue_1294
branch
2 times, most recently
from
f2d4462 to
3516431
Compare
Collaborator
Author
Collaborator
devin-petersohn
left a comment
devin-petersohn
left a comment
There was a problem hiding this comment.
@dchigarev Do we want this on 0.11? I think it would be good to add it there.
modin/pandas/test/utils.py
Outdated
Collaborator
There was a problem hiding this comment.
Will this hide bugs related to index name?
Collaborator
Author
Yes, I'd also like to have this in 0.11, I'll add this issue to the release tracker when it'll be created |
dchigarev
force-pushed
the
issue_1294
branch
from
36189a6 to
143e763
Compare
Collaborator
Author
|
@gshimansky @modin-project/modin-core all the comments have been addressed, PR is ready for review |
gshimansky
previously approved these changes
Sep 2, 2021
| # TODO: uncomment the following lines when #3331 issue will be closed | ||
| # @prepend_to_notes( | ||
| # """ | ||
| # In comparison with pandas, Modin's ``value_counts`` returns Series with ``MultiIndex`` |
Collaborator
There was a problem hiding this comment.
I am starting to think that we need to have some additional "compatibility mode" option in Modin that would try to reproduce the closest possible to Pandas behavior even at an expense of performance. For example #3298 also introduces a known difference, but in that case it is very subtle. In this case an object of different class (Index instead of MultiIndex) may really break some user's application when they replace import pandas with import modin.pandas.
What do you think if adding some MODIN_MAXIMUM_PANDAS_COMPAT option to enable workarounds for such differences?
Possibly this shouldn't be discussed in this PRs comments and brought somewhere outside.
Collaborator
|
@gshimansky , could you file an issue to discuss your suggestion there? |
vnlitvinov
previously approved these changes
Sep 8, 2021
Collaborator
vnlitvinov
left a comment
vnlitvinov
left a comment
There was a problem hiding this comment.
Overall LGTM, but I have a few questions.
Signed-off-by: Dmitry Chigarev <dmitry.chigarev@intel.com>
dchigarev
dismissed stale reviews from vnlitvinov, gshimansky, and YarShev
via
1dfc27b
…o issue_1294 Signed-off-by: Dmitry Chigarev <dmitry.chigarev@intel.com>
Collaborator
|
@devin-petersohn could you please have a look? |
Collaborator
devin-petersohn
left a comment
devin-petersohn
left a comment
There was a problem hiding this comment.
A minor style thing and a question. Looks great!
| if sort: | ||
| counted_values.sort_values(ascending=ascending, inplace=True) | ||
| if normalize: | ||
| counted_values = counted_values / counted_values.sum() |
Collaborator
There was a problem hiding this comment.
Slight style preference here:
Suggested change
| counted_values = counted_values / counted_values.sum() | |
| return counted_values / counted_values.sum() |
Collaborator
Author
There was a problem hiding this comment.
can't agree with this because there potentially could be some extra logic of postprocessing the final result
modin/pandas/test/utils.py
Outdated
| # why this clumsy if-statement is used. | ||
| res.index = res.index.droplevel(0) | ||
| res.name = series.name | ||
| # GroupBy discards original index names, so restoring them here |
Collaborator
There was a problem hiding this comment.
Why did we do this in the test? Can we do this in the code to make sure this will always pass?
Collaborator
Author
There was a problem hiding this comment.
removed code regarding to the Series name restoring and suddenly all tests have passed.
As for the index names, we can't do this names restoring in the GroupBy code because its natural behaviour is to set index names with the 'by' labels. Since the logic in this function is just about preprocessing frame before its comparison we want to index names be untouched, that's why we need this names restoring here.
I've clarified this at the in-code comment
Signed-off-by: Dmitry Chigarev <dmitry.chigarev@intel.com>
Collaborator
devin-petersohn
left a comment
devin-petersohn
left a comment
There was a problem hiding this comment.
@dchigarev Looks good. Can you clear the conflicts?
Signed-off-by: Dmitry Chigarev <dmitry.chigarev@intel.com>
Collaborator
Author
|
@devin-petersohn conflicts were solved and CI is green |
devin-petersohn
approved these changes
Sep 17, 2021
Collaborator
devin-petersohn
left a comment
devin-petersohn
left a comment
There was a problem hiding this comment.
Thanks @dchigarev, LGTM
Collaborator
|
Needs review from @modin-project/modin-omnisci |
Garra1980
approved these changes
Sep 21, 2021
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
7 participants
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.
Signed-off-by: Dmitry Chigarev dmitry.chigarev@intel.com
What do these changes do?
This PR moves
value_countsimplementation to the API level, technicallyvalue_countsisgroupby().size(), so this is the way it was implemented at the front-end.Side-effects:
In addition to
groupbyitself, there are some more steps to cover thevalue_countsparameters, all of these steps can't be done via groupby, so they're applied to the counted values separately from groupby. These separate applying generates up to 3 extra backend-API calls, since there is no query optimizer (for pandas backend) that would pack multiple calls into a single one we're getting an extra overhead of potential resplitting the frame several times during a singlevalue_countscall (more info at this comment).This new behavior leads to a performance regression in certain cases (for pandas backend only), but as it was agreed, we're okay with this for now (we believe in a bright future with a decent query optimizer).
Performance changes:
Here are the results of
ASVbenchmarks, they were run on 24 cores with a "Big" dataset, #2730 branch against the current master.Run command:
I manually added
sort=Falsecase, becausesort_values()is currently defaulting to pandas and so we don't want to compare defaulting-to-pandas vs non-defaulting-to-pandas.ASV results
Expanded timings and comparison with pandas are posted here
Formal todo list:
flake8 modinblack --check modingit commit -s