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

Make polygon fixture data valid #1472

Merged
merged 14 commits into from
Oct 17, 2024

Conversation

harrism
Copy link
Member

@harrism harrism commented Oct 10, 2024

Description

Fixes #1471. Fixes the invalid polygon geometry and improves geometry slicing tests to not depend on hard-coded vertex positions that are also hard-coded in the fixture.

Checklist

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

@github-actions github-actions bot added the Python Related to Python code label Oct 10, 2024
@harrism harrism self-assigned this Oct 10, 2024
@harrism harrism added bug Something isn't working non-breaking Non-breaking change labels Oct 10, 2024
@isVoid
Copy link
Contributor

isVoid commented Oct 10, 2024

For test_geometry_polygon_slicing, the solution to extract the coordinates (x, y and xy):

from shapely import get_coordinates
from itertools import chain

geom = gs[:9]
polys = geom[geom.apply(lambda x: isinstance(x, (MultiPolygon, Polygon)))]
coords = list(chain(polys.apply(get_coordinates)))

xy = list(chain(*coords))
x = [c[0] for c in coords]
y = [c[1] for c in coords]

@harrism harrism marked this pull request as ready for review October 15, 2024 05:31
@harrism harrism requested a review from a team as a code owner October 15, 2024 05:31
@harrism
Copy link
Member Author

harrism commented Oct 15, 2024

CI is blocked waiting for #1475

python/cuspatial/cuspatial/tests/conftest.py Outdated Show resolved Hide resolved
python/cuspatial/cuspatial/tests/test_geoseries.py Outdated Show resolved Hide resolved
python/cuspatial/cuspatial/tests/test_geoseries.py Outdated Show resolved Hide resolved
python/cuspatial/cuspatial/tests/test_geoseries.py Outdated Show resolved Hide resolved
Comment on lines 187 to 194
polys_list = gs[gs.apply(lambda x: isinstance(x, (MultiPolygon, Polygon)))]
polys = list(
chain(polys_list.apply(get_coordinates))
) # flatten multigeometries
coords_list = list(chain(*polys)) # flatten geometries
xy_interleaved = list(chain(*coords_list)) # flatten coordinates
x = xy_interleaved[::2]
y = xy_interleaved[1::2]
Copy link
Contributor

Choose a reason for hiding this comment

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

Not something we need to fix in this PR, but it would be nice to abstract this into a helper.

Copy link
Member Author

Choose a reason for hiding this comment

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

Agree, there is a lot of repetition.

Copy link
Member Author

Choose a reason for hiding this comment

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

Working on this.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done in 5488ac6

Copy link
Contributor

Choose a reason for hiding this comment

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

I assume you mean 1e96184? This looks much cleaner. Thanks!

@bdice
Copy link
Contributor

bdice commented Oct 16, 2024

I merged in the upstream to get CI fixes from #1475.

@harrism
Copy link
Member Author

harrism commented Oct 16, 2024

I merged in the upstream to get CI fixes from #1475.

Thanks. I take it you approve this PR? :)

Copy link
Contributor

@bdice bdice left a comment

Choose a reason for hiding this comment

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

Looks good to me.

@harrism harrism added the 5 - DO NOT MERGE Hold off on merging; see PR for details label Oct 17, 2024
@harrism harrism removed the 5 - DO NOT MERGE Hold off on merging; see PR for details label Oct 17, 2024
@harrism
Copy link
Member Author

harrism commented Oct 17, 2024

/merge

@rapids-bot rapids-bot bot merged commit 1bf2527 into rapidsai:branch-24.12 Oct 17, 2024
75 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working non-breaking Non-breaking change Python Related to Python code
Projects
Status: In Progress
Development

Successfully merging this pull request may close these issues.

[BUG]: GeoSeries tests use invalid data and rely on hard-coded data matching
3 participants