-
Notifications
You must be signed in to change notification settings - Fork 358
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 sort_values for Index/MultiIndex #1120
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1120 +/- ##
==========================================
+ Coverage 95.19% 95.19% +<.01%
==========================================
Files 35 35
Lines 7071 7075 +4
==========================================
+ Hits 6731 6735 +4
Misses 340 340
Continue to review full report at Codecov.
|
databricks/koalas/indexes.py
Outdated
if isinstance(self, MultiIndex): | ||
result.names = self.names | ||
else: | ||
result.name = self.name |
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.
Do we need 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.
without this, i think we can't keep names
when below case.
>>> kidx = ks.MultiIndex.from_tuples([('a', 'x', 1), ('c', 'y', 2), ('b', 'z', 3)])
>>> kidx.names = ['A', 'B', 'C']
>>> kidx.sort_values()
MultiIndex([('a', 'x', 1),
('b', 'z', 3),
('c', 'y', 2)],
)
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.
ah, I see. @names.setter
seems wrong.
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.
ah, i got it.
in @names.setter
, the new index_map
is overwritten to self._kdf._internal
, not to self._internal
.
like below
self._kdf._internal = internal.copy(index_map=list(zip(internal.index_columns, names)))
at this point, i curious why we overwrite self._kdf._internal
rather than simply self._internal
?
For now, i've fixed it to the current implementation
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 but let me leave it to @ueshin
Softagram Impact Report for pull/1120 (head commit: 8ae7722)
|
@itholic can you resolve conflicts? |
@HyukjinKwon resolved :) |
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.
LGTM.
Implement sort_values for Index/MultiIndex
(https://pandas.pydata.org/pandas-docs/stable/reference/api/pandas.Index.sort_values.html#pandas.Index.sort_values)
Support for MultiIndex.