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.distance breaks with new return type of GeoInterface.getgeom #419

Open
henrik-wolf opened this issue Mar 19, 2024 · 2 comments

Comments

@henrik-wolf
Copy link

I have some code which broke when the return type of getgeom changed in #369.

A mwe of the problem would be:

julia> using GeoInterface

julia> using ArchGDAL

julia> my_linestring = ArchGDAL.createlinestring([(0.0, 0.0), (1.0, 1.0)])
Geometry: LINESTRING (0 0,1 1)

julia> GeoInterface.distance(getgeom(my_linestring, 1), my_linestring)
ERROR: MethodError: no method matching distance(::PointTrait, ::LineStringTrait, ::Tuple{Float64, Float64}, ::ArchGDAL.IGeometry{ArchGDAL.wkbLineString})

I guess this could be fixed by adding the relevant dispatches to ArchGDAL in which we convert the tuple to a point which might be fine for this application? (The more general question of "how to handle these functions when combining different GeoInterface enabled packages" feels fairly complicated...)

@asinghvi17
Copy link
Contributor

asinghvi17 commented Apr 4, 2024

To your second point, we're writing https://www.github.com/asinghvi17/GeometryOps.jl which is meant to provide Julia-native algorithms and a unified API for these kinds of geometry operations.

In general the dispatch could be added, but then you would get into adding a dispatch for each combination which seems a bit difficult...

@rafaqz
Copy link
Collaborator

rafaqz commented Apr 5, 2024

Honestly I have pretty much given up on GeoInterface.distance and similar methods being something that can work in practice. We should really deprecate them, but in this case I didn't intend for returning a point to break them (ArchGDAL objects is one of the only use cases where they actually worked).

One easy fix is to return another Point type that just wraps the tuple but ArchGDAL owns and can dispatch on. Anything is better than the old C object Point that needs a finaliser (imagine having a million of those...).

This was meant to be a/the alternative:
#366

But I pretty much stalled on fixing ArchGDAL geoms because its so hard to work on and after everything it is still really slow, especially without the point fix that caused this problem.

But, we should return a struct point instead of a Tuple. If you want to PR its probably only 10 lines.

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

No branches or pull requests

3 participants