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

Geointerface conversions #156

Merged
merged 8 commits into from
Apr 22, 2023
Merged

Geointerface conversions #156

merged 8 commits into from
Apr 22, 2023

Conversation

rafaqz
Copy link
Member

@rafaqz rafaqz commented Jan 28, 2023

This PR adds the new geointerface_geomtype local methods, updates convert to not use coordinates, and adds methods to all functions that will work on any compatible geometries.

There are some extra methods needed for some constructors to make convert more consistent.

Its not really practical to test this without wrapper types, the test objects are too limited:

So also waiting on JuliaGeo/GeoInterface.jl#78

We may as well review this now as its basically ready to go, just needs 78 to work. @jw3126 if you would also like to review this that would be helpful.

@rafaqz rafaqz changed the title start on geointerface conversions Geointerface conversions Jan 28, 2023
@rafaqz rafaqz requested a review from visr February 15, 2023 15:20
src/geo_interface.jl Outdated Show resolved Hide resolved
src/geo_interface.jl Outdated Show resolved Hide resolved
@rafaqz rafaqz force-pushed the rs/geointerface_conversions branch from 35c8ee3 to e3e716d Compare February 22, 2023 00:36
@rafaqz rafaqz force-pushed the rs/geointerface_conversions branch from e3e716d to 8e8c788 Compare March 27, 2023 18:39
@rafaqz rafaqz marked this pull request as ready for review March 27, 2023 18:39
@rafaqz rafaqz changed the title Geointerface conversions Breaking: Geointerface conversions Mar 27, 2023
@rafaqz
Copy link
Member Author

rafaqz commented Mar 27, 2023

This should be good to go. It's a breaking change because I have renamed Base.contains to LibGEOS.contains, otherwise we can't accept other geometries in it without piracy.

We can lump it with #155

I made this non breaking to reduce friction, there are now both Base.contains and LibGEOS.contains methods. We can remove the base method when we do #155

@rafaqz rafaqz requested a review from evetion March 28, 2023 10:28
@rafaqz rafaqz changed the title Breaking: Geointerface conversions Geointerface conversions Mar 28, 2023
@rafaqz
Copy link
Member Author

rafaqz commented Apr 10, 2023

Bump

@visr @evetion @jw3126 can I get a review for this?

Copy link
Member

@visr visr left a comment

Choose a reason for hiding this comment

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

Left one suggestion, but this looks good to me! This will be great to have.

It would be nice if we could avoid the .ptr added in this PR, but it seems like that is probably better addressed separately.

test/test_geo_interface.jl Outdated Show resolved Hide resolved
@rafaqz
Copy link
Member Author

rafaqz commented Apr 22, 2023

Yes I was thinking about adding a better internal getx etc method, but it can be separate.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants