JuliaGeo / GeoInterface.jl

A Julia Protocol for Geospatial Data
https://juliageo.org/GeoInterface.jl/stable/
MIT License
90 stars 15 forks source link

Deprecate predicates and functions like `area` #127

Open rafaqz opened 4 months ago

rafaqz commented 4 months ago

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)

evetion commented 4 months ago

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.

asinghvi17 commented 4 months ago

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.

evetion commented 4 months ago

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".

asinghvi17 commented 4 months ago

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.

rafaqz commented 4 months ago

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.