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

FIX/MNT: Simplify project geometry handling #2407

Merged
merged 1 commit into from
Jul 8, 2024

Conversation

greglucas
Copy link
Contributor

The MultiLineString return type was a plain list, but it should be an empty MultiLineString to be consistent with the other return types. Additionally, all geometry constructors take empty lists, so just use that rather than special-casing the returns.

Closes #2406

@greglucas
Copy link
Contributor Author

Interesting, the geom_type has changed from GeometryCollection to MultiLineString. So previously, this would have failed before even making it into the transformation because we don't know how to project GeometryCollections. Should we ignore this test with shapely<2, or other suggestions for how to handle it?

The MultiLineString return type was a plain list, but it should be
an empty MultiLineString to be consistent with the other types.
Additionally, all geometry constructors take empty lists, so
just use that rather than special-casing the returns.
@greglucas greglucas force-pushed the multi-line-return branch from d8fcd1d to c4f2087 Compare June 29, 2024 18:41
@dopplershift
Copy link
Contributor

I think it's important to know whether any of those classes accepted empty lists on Shapely < 2? It's not clear to me whether the test is checking code paths that we could even hit before in practice.

Essentially, after this change: do we actually still support Shapely < 2?

@greglucas
Copy link
Contributor Author

Yes, we still have a minimum test that pulls in Shapely 1.8.

https://github.com/SciTools/cartopy/actions/runs/9726193384/job/26844355302#step:4:1

Shapely 2.0 changes the geom_type return to be more correct. Not a regression for us, just additional things that will get projected properly now that we get for free. It just impacts the test where this fails without shapely 2.0 and was also failing before this update.

Copy link
Contributor

@dopplershift dopplershift left a comment

Choose a reason for hiding this comment

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

This looks good to me then.

@dopplershift dopplershift added this to the Next Release milestone Jul 8, 2024
@dopplershift dopplershift merged commit ca2832e into SciTools:main Jul 8, 2024
21 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Projection._project_multiline() returns empty list rather than empty sgeom.MultiLineString
2 participants