search

yeesian / ArchGDAL.jl

A high level API for GDAL - Geospatial Data Abstraction Library
https://yeesian.github.io/ArchGDAL.jl/stable/
Other
137 stars 25 forks source link

`GeoInterface.distance` breaks with new return type of `GeoInterface.getgeom` #419

Open henrik-wolf opened 3 months ago

henrik-wolf commented 3 months ago

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 commented 2 months ago

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 commented 2 months ago

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: https://github.com/yeesian/ArchGDAL.jl/pull/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.