-
Notifications
You must be signed in to change notification settings - Fork 362
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Implement Series.truncate #928
Conversation
Codecov Report
@@ Coverage Diff @@
## master #928 +/- ##
=========================================
+ Coverage 94.49% 94.5% +0.01%
=========================================
Files 34 34
Lines 6434 6447 +13
=========================================
+ Hits 6080 6093 +13
Misses 354 354
Continue to review full report at Codecov.
|
databricks/koalas/series.py
Outdated
e 50 | ||
Name: 0, dtype: int64 | ||
""" | ||
indexes = self.index.to_pandas() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@itholic, this will collect everything in index into driver's memory. We should avoid this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Koalas index implements is_monotonic_increasing
and is_monotonic_decreasing
. The problem is that this is an expensive operation as warned in their documentations as it leads all data into single node.
Can you add a note that this API will be expensive (see is_monotonic_increasing
as an example). We even might have to explicitly don't implement. cc @ueshin
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@HyukjinKwon Thanks for commenting!
yeah you're right. i used is_monotonic_increasing
of pandas' because there was a bug for koalas'. (so i fixed in #930 )
now we can use koalas', but there still have an issue about cost as you said.
and first, i wrote note about warning in this API docstring as you said
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks fine to me. I'll take it back.
Softagram Impact Report for pull/928 (head commit: 558d978)⭐ Change Overview
📄 Full report
Impact Report explained. Give feedback on this report to [email protected] |
Like pandas Series.truncate (https://pandas.pydata.org/pandas-docs/version/0.21/generated/pandas.Series.truncate.html#pandas.Series.truncate)
implemented function
truncate
for Series.