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

Add reset_index to GeoSeries and GeoDataFrame #856

Merged
merged 15 commits into from
Dec 15, 2022

Conversation

thomcom
Copy link
Contributor

@thomcom thomcom commented Dec 13, 2022

Closes #849

Description

This PR adds .reset_index to GeoSeries and GeoDataFrame. It tests GeoDataFrame to the same level that cudf tests the method. However GeoSeries has four blocks of parameterized tests numbering in the hundreds. I only implement the first test block for GeoSeries.

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 added bug Something isn't working 3 - Ready for Review Ready for review by team non-breaking Non-breaking change labels Dec 13, 2022
@thomcom thomcom requested a review from a team as a code owner December 13, 2022 21:37
@thomcom thomcom self-assigned this Dec 13, 2022
@thomcom thomcom requested a review from vyasr December 13, 2022 21:37
@github-actions github-actions bot added the Python Related to Python code label Dec 13, 2022
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.

I think reset_index for GeoSeries can be optimized a bit. But too late here.. Just sigining off for first round.

python/cuspatial/cuspatial/core/geodataframe.py Outdated Show resolved Hide resolved
python/cuspatial/cuspatial/core/geodataframe.py Outdated Show resolved Hide resolved
python/cuspatial/cuspatial/core/geodataframe.py Outdated Show resolved Hide resolved
python/cuspatial/cuspatial/core/geodataframe.py Outdated Show resolved Hide resolved
python/cuspatial/cuspatial/core/geodataframe.py Outdated Show resolved Hide resolved
if not drop and inplace:
pytest.skip(
"For exception checks, see "
"test_reset_index_dup_level_name_exceptions"
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this just a left over from cudf?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is copied over from cudf, yes. I could rewrite the parameterizations so that this invalid case never occurs. If drop == False and inplace == True for a series reset_index an exception is thrown. Since the result has to be a GeoDataFrame, inplace is impossible. cudf just built a matrix of parameterizations and skips the impossible ones.

Copy link
Contributor

Choose a reason for hiding this comment

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

I see, if it's expected to fail, instead of skipping, should we just wrap it in pytest.raises block?

@thomcom thomcom requested review from harrism and isVoid and removed request for vyasr December 14, 2022 14:41
@thomcom thomcom added 4 - Needs Reviewer Waiting for reviewer to review or respond and removed 3 - Ready for Review Ready for review by team labels Dec 15, 2022
@thomcom
Copy link
Contributor Author

thomcom commented Dec 15, 2022

@gpucibot merge

@rapids-bot rapids-bot bot merged commit 73e2a27 into rapidsai:branch-23.02 Dec 15, 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 bug Something isn't working non-breaking Non-breaking change Python Related to Python code
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

[BUG]: .reset_index() doesn't work with GeoSeries anymore
2 participants