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

Enable pytest failures on warnings on FutureWarnings (Replace deprecated geopandas.dataset module) #1360

Merged
merged 10 commits into from
Apr 16, 2024

Conversation

mroeschke
Copy link
Contributor

Description

Continuing the effort in rapidsai/build-planning#26 by enabling FutureWarnings and DeprecationWarnings to raise errors in the CI.

The primary change here is to replace geopandas.dataset usage with the files used (the data in https://github.com/geopandas/geopandas/tree/main/geopandas/tests/data)

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 March 13, 2024 21:17
@github-actions github-actions bot added the Python Related to Python code label Mar 13, 2024
gpu_dataframe = cuspatial.from_geopandas(host_dataframe)
# The df size is 8kb of cudf rows and 217kb of the geometry column
assert gpu_dataframe.memory_usage().sum() == 224945
assert gpu_dataframe.memory_usage().sum() == 216793
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This test is now essentially the same as test_memory_usage_large below. Is it OK to deduplicate?

Copy link
Member

Choose a reason for hiding this comment

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

It's the same size because there are no geoseries in the geodataframe other than "geometry", right? The logic is different.

Copy link
Contributor Author

@mroeschke mroeschke Mar 22, 2024

Choose a reason for hiding this comment

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

Ah yes you're correct. gpd.read_file(gpd.datasets.get_path("naturalearth_lowres")) also has pop_est, continent, name, iso_a3, and gdp_md_est columns. Is it OK if this test no longer has those columns?

Copy link
Member

Choose a reason for hiding this comment

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

I don't know. @isVoid @thomcom do you?

Copy link
Contributor

Choose a reason for hiding this comment

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

This does change the logic, previously the test was also validating that the cudf series are being read with from_geopandas. I don't think another test verifies this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry coming back to this now.

cudf series

Did you mean geopandas series? It looks like that may be tested in python/cuspatial/cuspatial/tests/test_from_geopandas.py and I don't see cuspatial.from_geopandas supporting a cudf.Series

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.

Thanks for finally fixing the datasets warnings.

gpu_dataframe = cuspatial.from_geopandas(host_dataframe)
# The df size is 8kb of cudf rows and 217kb of the geometry column
assert gpu_dataframe.memory_usage().sum() == 224945
assert gpu_dataframe.memory_usage().sum() == 216793
Copy link
Member

Choose a reason for hiding this comment

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

It's the same size because there are no geoseries in the geodataframe other than "geometry", right? The logic is different.

@mroeschke mroeschke requested a review from a team as a code owner March 22, 2024 18:01
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@github-actions github-actions bot added conda Related to conda and conda configuration ci labels Mar 22, 2024
@mroeschke mroeschke changed the base branch from branch-24.04 to branch-24.06 March 22, 2024 18:01
@github-actions github-actions bot removed conda Related to conda and conda configuration ci labels Apr 2, 2024
@vyasr vyasr added improvement Improvement / enhancement to an existing function non-breaking Non-breaking change labels Apr 16, 2024
@vyasr
Copy link
Contributor

vyasr commented Apr 16, 2024

/merge

@rapids-bot rapids-bot bot merged commit c5203dc into rapidsai:branch-24.06 Apr 16, 2024
69 checks passed
@mroeschke mroeschke deleted the testing/future/warnings branch April 16, 2024 21:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
improvement Improvement / enhancement to an existing function non-breaking Non-breaking change Python Related to Python code
Projects
Status: Todo
Development

Successfully merging this pull request may close these issues.

6 participants