Skip to content

Conversation

rafaqz
Copy link
Member

@rafaqz rafaqz commented Mar 28, 2023

This PR adds some basic GeoInterface.jl compatability for the Coords object, and uses that to add a reproject method that can reproject any geointerface geometry between source and target crs.

reconstruct could be moved to GeoInterface as it's quite generic - it rebuilds any nested geometry from a vector of points using wrapper types that match the original structure. Currently it's not type stable.

One issue is that GeoInterface.ncoord doesn't work on Coords - currently it returns 4 because its an array with x y z and timestamp, but there is no way we can detect if it is 2 or 3.

We may have to settle for ncoord always being 3, and changing how the GeoInterface wrappers check point objects - we can let them have extra coords and use the wrapper to clamp them to hide the z. Currently the wrapper Point checks the number of coords in the wrapped objects and errors if it doesn't match.

Another option is to add a type parameter to Coord to indicate if it is 2 or 3 dimensional.

The returned object will be constructed from `GeoInterface.WrapperGeometry`
geometries, wrapping views of `Proj.Coord`.
"""
function reproject(geom, source_crs, target_crs; time=Inf)
Copy link
Member

Choose a reason for hiding this comment

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

Can we add this as a method stub to GeoInterface so that other packages (ArchGDAL?) can specialize 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.

Does that make sense though? It wouldn't really give us anything more than we have.

The point of this PR is that it converts any geointerface object using Proj. There is no way to dispatch on that, so a stub would necessitate type piracy. I already wrote a ArchGDAL.reproject method years ago. It will also handle any GeoInterface geometries when this is merged:
yeesian/ArchGDAL.jl#366

But you have to use ArchGDAL.reproject for that to work. If we use a stub method in GeoInterface.jl then the packages can only handle types they own, so ArchGDAL.AbstractGeometry for ArchGDAL.jl, and nothing for Proj.jl.

I'm trying to shift everything to package methods because the methods in GeoInterface (like area etc) don't really make sense, there is no way to add useful methods to them without piracy or a backend system. Mostly I use Shapefile.jl and GeoJSON.jl directly, and need a way to work with those objects.

Copy link
Member

@asinghvi17 asinghvi17 Apr 3, 2023

Choose a reason for hiding this comment

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

I see what you're saying. Packages could implement functions which take arbitrary GeoInterface geometries and return their own internal structs then?

Copy link
Member Author

@rafaqz rafaqz Apr 3, 2023

Choose a reason for hiding this comment

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

Yep. That's what my PRs in LibGEOS, ArchGDAL and this one are doing.

I'm trying to make them at least be the same name and syntax in all packages.

For example reproject should be the essentially the same in ArchGDAL, Rasters and Proj. (Rasters will need some changes)

@rafaqz rafaqz marked this pull request as draft March 29, 2023 22:03
@rafaqz rafaqz marked this pull request as ready for review April 27, 2023 20:57
@rafaqz
Copy link
Member Author

rafaqz commented Apr 28, 2023

@visr @evetion I think this is good to go, if you would like to review.

The idea was to use syntax close to ArchGDAL.reproject but to also update it for keywords to match this PR.

We could also define trans(geom) to work in a similar way if that makes sense for Transformations?

@rafaqz rafaqz requested review from evetion and visr April 28, 2023 17:41
@rafaqz rafaqz closed this May 4, 2023
@rafaqz
Copy link
Member Author

rafaqz commented May 4, 2023

Closed in favour of putting this in GeometryOps.jl

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