-
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 DataFrame/Series reindex_like #1880
Conversation
Nice work, @LucasG0 ! |
Codecov Report
@@ Coverage Diff @@
## master #1880 +/- ##
=======================================
Coverage 94.19% 94.19%
=======================================
Files 40 40
Lines 9867 9939 +72
=======================================
+ Hits 9294 9362 +68
- Misses 573 577 +4
Continue to review full report at Codecov.
|
Despite Codecov report, coverage seems actually ok. |
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.
Otherwise, seems fine to me since it's basically based on specific usage of reindex
.
---------- | ||
other : Series or DataFrame | ||
Its row and column indices are used to define the new indices | ||
of this object. |
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.
Anyway, maybe pandas has copy
parameter for Series.reindex_like
, too ??
Even if it doesn't really do any meaningful work, how about adding it for compatibility with 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.
Yes pandas has copy
parameter for Series.reindex_like
. If we add it, we need to update Series.reindex
too, which does not support copy
parameter either.
Should I do the change ?
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.
Hmm... okay let's keep it as it is for now and discuss separately later.
Thanks for the opinion!
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.
Ok :)
awesome, thanks @LucasG0 for working on this. |
Great, @LucasG0 ! |
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.
Otherwise, LGTM.
Thanks! merging. |
Hi, this PR implements both
DataFrame.reindex_like
andSeries.reindex_like
.I did not add a test about reindexing MultiIndex columns on single Index columns because it is currently not supported in koalas. Also, as for
DataFrame.reindex
, there is no test for reindexing single Index columns/index on MultiIndex columns/index, as it is not supported in pandas.