Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Implement Series.reindex #1737

Merged
merged 1 commit into from
Sep 7, 2020
Merged

Implement Series.reindex #1737

merged 1 commit into from
Sep 7, 2020

Conversation

LucasG0
Copy link
Contributor

@LucasG0 LucasG0 commented Aug 30, 2020

This PR would close #881.
It includes moving the method DataFrame._reindex_index to the generic Frame class.

@LucasG0 LucasG0 mentioned this pull request Aug 30, 2020
Closed
@LucasG0 LucasG0 force-pushed the reindex branch 3 times, most recently from 23e42d7 to 57572d0 Compare August 30, 2020 22:28
itholic
itholic reviewed Aug 30, 2020
View reviewed changes
databricks/koalas/tests/test_series.py Outdated Show resolved Hide resolved
databricks/koalas/tests/test_series.py Outdated Show resolved Hide resolved
databricks/koalas/tests/test_series.py Outdated Show resolved Hide resolved
@itholic
Copy link
Contributor

itholic commented Aug 30, 2020

Basically, the logic seems fine to me.
But I think we better reposition of some codes. It's looks a bit of complicated.

@itholic itholic requested review from ueshin and HyukjinKwon August 30, 2020 23:03
@itholic
Copy link
Contributor

itholic commented Aug 30, 2020

Anyway, thanks for the work on this, @LucasG0 :)

@LucasG0 LucasG0 force-pushed the reindex branch 2 times, most recently from 47b088c to 2ac9745 Compare August 31, 2020 12:10
@LucasG0
Copy link
Contributor Author

LucasG0 commented Aug 31, 2020 •
edited
Loading

Thanks for reviewing !
Ideally we could avoid code duplicate with _reindex_index generic. I submited a new version without type checking in which Series constructor handles InternalFrame. It may be worth introducing this feature for this issue, as It could be reused for other purposes. I am interested in your opinion on this.
If it still looks complicated, I will probably just separate DataFrame.reindex and Series.reindex. :)

@itholic
Copy link
Contributor

itholic commented Sep 3, 2020 •
edited
Loading

Sorry for the late, @LucasG0 .
Seems not bad, but I'm not sure it's good enough reason to modify the constructor at this point.
@ueshin @HyukjinKwon , WDYT ??

@LucasG0 LucasG0 force-pushed the reindex branch 2 times, most recently from 4100d8b to 3defa8c Compare September 6, 2020 20:31
itholic
itholic approved these changes Sep 7, 2020
View reviewed changes
Copy link
Contributor

@itholic itholic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Otherwise, LGTM

databricks/koalas/series.py Outdated Show resolved Hide resolved
databricks/koalas/series.py Outdated Show resolved Hide resolved
@LucasG0 LucasG0 force-pushed the reindex branch 2 times, most recently from c5cf742 to ba105e6 Compare September 7, 2020 09:14
@itholic itholic merged commit eca08f1 into databricks:master Sep 7, 2020
@itholic
Copy link
Contributor

itholic commented Sep 7, 2020

Thanks, @LucasG0 ! Merging :)

@LucasG0
Copy link
Contributor Author

LucasG0 commented Sep 7, 2020

Thanks. :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@itholic itholic itholic approved these changes

@HyukjinKwon HyukjinKwon HyukjinKwon approved these changes

@ueshin ueshin Awaiting requested review from ueshin

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Series.reindex
3 participants
@LucasG0 @itholic @HyukjinKwon