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

Disallow GeoSeries from accepting a column in favor of _from_column #1434

Merged
merged 5 commits into from
Aug 9, 2024

Conversation

mroeschke
Copy link
Contributor

Description

closes #1433

Mirroring the upstream cudf changes in rapidsai/cudf#16454, cudf.Series disallows accepting a ColumnBase in favor of the cudf.Series._from_column constructor. This PR does the same for GeoSeries as well as addresses the breakages due to the upstream cudf change.

Checklist

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

@mroeschke mroeschke requested a review from a team as a code owner August 7, 2024 17:35
@mroeschke mroeschke changed the title Disallow GeoSeries from accepting a column in favor of _from_column Disallow GeoSeries from accepting a column in favor of _from_column Aug 7, 2024
@github-actions github-actions bot added the Python Related to Python code label Aug 7, 2024
@harrism harrism added bug Something isn't working breaking Breaking change labels Aug 7, 2024
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.

Question: are cudf.Series._* methods considered private? If so, it makes me nervous to add more private cudf access in cuspatial.

@bdice
Copy link
Contributor

bdice commented Aug 7, 2024

Question: are cudf.Series._* methods considered private? If so, it makes me nervous to add more private cudf access in cuspatial.

I don't think there is an alternative path here. Previously, constructing a Series from a column was not a code path that we intended to be public, but it worked. @mroeschke took that away to reduce the abuse of the Series constructor. If I understand the plan correctly, in the future all the code using columns (private internals of cudf) should be moved to use pylibcudf where columns will have a stable public API. That public API is not yet ready, to my knowledge. In the interim, this PR will be necessary to unblock cuspatial.

@bdice
Copy link
Contributor

bdice commented Aug 7, 2024

Looks like some CI tests are failing:

FAILED tests/test_transform.py::test_geoseries_input - TypeError: data type 'geometry' not understood

@bdice
Copy link
Contributor

bdice commented Aug 9, 2024

/merge

@rapids-bot rapids-bot bot merged commit 1b60fb7 into rapidsai:branch-24.10 Aug 9, 2024
75 checks passed
@mroeschke mroeschke deleted the fix/series/column branch August 9, 2024 17:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking Breaking change bug Something isn't working Python Related to Python code
Projects
Status: Review
Development

Successfully merging this pull request may close these issues.

[BUG]: cannot construct a GeoSeries
3 participants