GeoInterface.jl icon indicating copy to clipboard operation
GeoInterface.jl copied to clipboard

Deprecate predicates and functions like `area`

Open rafaqz opened this issue 1 year ago • 5 comments

In practice these don't actually work (widely), because of dispatch. As far as I know only ArchGDAL and LibGEOS objects have both the methods and the objects to provide the dispatch.

A recent slack thread pointed out this problem:

ERROR: MethodError: no method matching contains(::PolygonTrait, ::PointTrait, ::GeoJSON.Polygon{2, Float64}, ::GeoJSON.Point{2, Float64})

GeoJSON doesn't have contains and likely never will, and shouldn't depend on a package that does have it. So we are stuck with the error.

What GeoInterface does very well at this stage is let LibGEOS.jl and GeometryOps.jl provide their own predicates that work on any type that follows the GI interface. GeometryOps.contains or LibGEOS.contains will already just work with GeoJSON.jl geoms with no dependency or special treatment.

Unless we think of some other solution I suggest we deprecate these methods to avoid confusing users, and focus on the object interface that lets other packages define them.

(I also think this part of GI not working is a distraction from just how well the other parts of the interface work)

rafaqz avatar Apr 17 '24 09:04 rafaqz

Can we de-deprecate them later on? I would like that when we have a blessed fallback in GeometryOps? A "blessed" implementatation means accepting it does pirate some times, we have seen similar things with WellKnownGeometry and GeoFormatTypes.

evetion avatar Apr 17 '24 10:04 evetion

GeometryOps already implements everything except buffer, convex hull, relate, and symdiff. I guess GeometryOps would define generic dispatches for all trait combinations, and then any GI consumer package can handle its own geometry?

We would have to ensure there are no signatures like:

AG.within(::PolygonTrait, ::PointTrait, ::IGeometry, ::Any)

to avoid ambiguities when ArchGDAL and LibGEOS are loaded together, for example.

asinghvi17 avatar Apr 17 '24 11:04 asinghvi17

Ideally (but I think the slowdown is maybe not acceptable) we could change these GeoInterface methods to give an extra warning on a MethodError: "Make an issue or do using GeometryOps".

evetion avatar Apr 17 '24 12:04 evetion

We can always add an error hint that checks whether GeometryOps is loaded in the current environment...I do something similar for reproject there. Checking Base.loaded_modules should work.

asinghvi17 avatar Apr 17 '24 12:04 asinghvi17

A blessed implementation is another option. The reason I didn't push for that is that I'm a little woried about the edge cases:

Like this is a problem:

GI.contains(gdalgeom, gdalgeom) -> ArchGDAL
GI.contains(libgeosgeom, libgeosgeom) -> LibGEOS
GI.contains(libgeosgeom, gdalgeom) -> GeometryOps

There will be a bunch or unpredictable behaviour like this where its not clear what package is doing what operation, where to take issues, who to trust.

rafaqz avatar Apr 17 '24 13:04 rafaqz