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.

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

Allow None rows in GeoSeries and an align method to match GeoPandas #760

Merged
merged 22 commits into from
Nov 28, 2022

Conversation

thomcom
Copy link
Contributor

@thomcom thomcom commented Oct 31, 2022

Description

This PR allows us to have GeoSeries with empty rows containing None. This feature is used by GeoPandas frequently with binary operations on GeoSeries of different lengths. It also adds an align method which works with default arguments for aligning the indices of a Series and adding None where rows are missing.

I also updated the internals for GeoColumn, GeoSeries, and GeoDataFrame to handle native _gather operations from cudf correctly. This eliminates the need to use column._data as a reordering map when sort_values is used on a non-geometry column, and should support more hidden cudf operations that we haven't attempted yet.

Closes #762
Closes #761

Checklist

  • I am familiar with the Contributing Guidelines.
  • New or existing tests cover these changes.
  • The documentation is up to date with these changes.

@thomcom thomcom requested review from a team as code owners October 31, 2022 22:17
@github-actions github-actions bot added libcuspatial Relates to the cuSpatial C++ library Python Related to Python code labels Oct 31, 2022
@thomcom thomcom self-assigned this Oct 31, 2022
@thomcom thomcom added 2 - In Progress Currenty a work in progress non-breaking Non-breaking change labels Oct 31, 2022
@thomcom thomcom added this to the DE-9IM milestone Oct 31, 2022
@thomcom thomcom added the improvement Improvement / enhancement to an existing function label Nov 1, 2022
@github-actions github-actions bot removed the libcuspatial Relates to the cuSpatial C++ library label Nov 1, 2022
@thomcom thomcom added 4 - Needs Reviewer Waiting for reviewer to review or respond and removed 2 - In Progress Currenty a work in progress labels Nov 1, 2022
@thomcom thomcom requested review from harrism and isVoid November 2, 2022 17:37
Copy link
Contributor

@isVoid isVoid left a comment

Choose a reason for hiding this comment

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

Thanks, lgtm.

Pending one (non-blocking) question about design here: #760 (comment)

@harrism
Copy link
Member

harrism commented Nov 7, 2022

rerun tests

Copy link
Member

@harrism harrism left a comment

Choose a reason for hiding this comment

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

I could use more explanation of "align".

Comment on lines 579 to 581
>>> polygons_right = gpd.GeoSeries([
Point((-2, -2)),
Point((-8, -8)),
Copy link
Member

Choose a reason for hiding this comment

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

Why is this called polygons_right if it contains Points, not Polygons?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy/pasto


def align(self, other):
"""
Align the rows of two GeoSeries using outer join.
Copy link
Member

Choose a reason for hiding this comment

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

What does it mean to align the rows of geoseries? I think a more detailed explanation would help.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will write one that covers this implementation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I increased the docs, explaining three use cases, and provided examples for them. Please approve and merge @harrism .

@harrism
Copy link
Member

harrism commented Nov 28, 2022

@gpucibot merge

@rapids-bot rapids-bot bot merged commit 9904cfb into rapidsai:branch-22.12 Nov 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4 - Needs Reviewer Waiting for reviewer to review or respond improvement Improvement / enhancement to an existing function non-breaking Non-breaking change Python Related to Python code
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

[FEA] GeoSeries needs to support None rows entries. [FEA] GeoSeries needs index alignment
3 participants