New (breaking) Traits based release

[evetion]

Based on https://github.com/JuliaGeo/GeoInterfaceRFC.jl

• Decide what to do about primitives.jl (removed it)
• Update all integrations in other packages (https://github.com/JuliaGeo/GeoInterfaceRFC.jl/blob/master/INTEGRATIONS.md)
  • ArchGDAL (Implement new GeoInterface traits yeesian/ArchGDAL.jl#290)
  • LibGEOS (blocked by getGeometry and getGeometries in geos operations LibGEOS.jl#101 and numGeometries in geos operations LibGEOS.jl#96)
  • GeometryBasics (commented on rewrite)
  • ShapeFile (Update for the new GeoInterface Shapefile.jl#72)
  • Rasters
  • GeoDataFrames
  • Meshes.jl
  • FlatGeobuf.jl (https://github.com/evetion/FlatGeobuf.jl)
  • GeoParquet.jl (depends on WellKnownGeometry)
  • WellKnownGeometry.jl (https://github.com/evetion/WellKnownGeometry.jl)
  • GeoJSONTables (https://github.com/visr/GeoJSONTables.jl)

@visr @yeesian @juliohm

[juliohm]

Awesome interface. Thank you for the effort again.

[rafaqz]

What is the status of this PR? It would be good to build GeoData.jl polygon operations off this rather than master.

[yeesian]

I was thrown off by the [WIP] in the title, but the implementation is great! Sorry for my slowness in reviewing this PR!

[rafaqz]

I've been reading over this too now I'm using GeoInterface more. It seems like a real improvement, I'm keen to help getting it merged.

My comments so far are:

• it seems that extent replaces bbox ? I can't find the implementation of extent
• crs should probably return nothing instead of missing by default. nothing is more often used like that.
• We will probably need more fallbacks that return nothing when called on an object that doesn't support the trait. I've just added these to the old GeoInterface for bbox and crs so we can call it just in case, as e.g. Shapefiles have bbox precalculated for everything, but other gemoetries may not.
• We should make sure that coordnames and x/yconstant-propagates and findfist isn't a runtime lookup.

[rafaqz]

Suggested change

	crs(geom) = missing # or conforming to <:CoordinateReferenceSystemFormat in GeoFormatTypes
	crs(geom) = nothing # or conforming to <:CoordinateReferenceSystemFormat in GeoFormatTypes

[rafaqz]

extent needs to be implemented.

Could it hold/be a NamedTuple ? Like extent(obj) = Extent((X=(xmin, xmax), Y=(ymin, ymax))) etc for how many dimensions there are, using the :X, :Y, :Z, :M names from the interface.

I find this organisation is easier to use than the xmin, ymin, xmax, ymax style bbox usually uses.

(but somehow I missed that its meant to return a Rectangle. I guess that makes sense in a different way.)

[rafaqz]

I would like to propose an Extent object to be used instead of Rectangle for the return value of extent. It wraps a NamedTuple because all fields are not always present.

struct Extent{T<:NamedTuple}
    bounds::T
end
Extent(; kw...) = Extent(values(kw))
Base.getproperty(ext::Extent, x::Symbol) = getproperty(getfield(ext, :bounds), x)

ext = Extent(X=(0.0, 10.0), Y=(0.0, 10.0))
ext.X

Giving it a specific type means we can dispatch on it, and including the dimension names means we know their order in the object.

[evetion]

Ah yes, I agree that we need some sort of bounds object. I've had issues finding a generic one, hence the default to a Rectangle. Nowadays I just revert to using a namedtuple, but am frustrated by the fact that a (min_x=1, min_y=2) != (min_y=2, min_x=1)(nor its types), which for all purposes would be equal.

@visr Do you remember my magical bbox code that you reviewed and we shelved for it being too complex? I can't seem to find it.

edit: Found it here JuliaGeo/GeoInterfaceRFC.jl@527198b#diff-64b0c51af7618fbd8cd973bf88690c8b22c309f9b718e4fdd41b7555769f23fdR25

[rafaqz]

Yeah, a NamedTuple is not enough. But its close to it... that's why I've just wrapped it with Extent. You get a lot of the code in your BoundingBox object for free just by forwarding to NamedTuple.

With the Extent wrapper we could define == to check that both objects have the same dimension names (but not necessarily the same order), and then match all of the values by the names.

[rafaqz]

Better implementation including ==

struct Extent{T<:NamedTuple}
    bounds::T
end
Extent(; kw...) = Extent(values(kw))

bounds(ext::Extent) = getfield(ext, :bounds)

Base.getproperty(ext::Extent, x::Symbol) = getproperty(bounds(ext), x)
Base.getindex(ext::Extent, x::Symbol) = getindex(bounds(ext), x)
import Base: ==
function (==)(a::Extent{<:NamedTuple{K1}}, b::Extent{<:NamedTuple{K2}}) where {K1, K2}
    length(K1) == length(K2) || return false
    all(map(k -> k in K1, K2)) || return false
    return all(map((k -> a[k] == b[k]), K1))
end

And the order doesn't matter for ==:

julia> Extent(X=(0, 1), Y=(3.0, 4.0)) == Extent(Y=(3.0, 4.0), X=(0, 1))
true

julia> @btime Extent(X=(0, 1), Y=(3.0, 4.0)) == Extent(Y=(3.0, 4.0), X=(0, 1))
  4.004 ns (0 allocations: 0 bytes)
true

[rafaqz]

Cautionary tale against using missing as a default return value in an interface:
JuliaArrays/ArrayInterface.jl#245

[evetion]

Thanks, that's good to know when updating this PR.

[evetion]

Some updates:

• Removed merge conflicts
• Added documentation (still more to do)
• Removed primitives, they could be added back (Vector/Tuple is a Point? but what about other geometries?)
• Added isgeometry check, mimicking istable from Tables.jl
• Enumerated current packages using GeoInterface

• it seems that extent replaces bbox ? I can't find the implementation of extent

Will now use Extents.jl once registered.

• crs should probably return nothing instead of missing by default. nothing is more often used like that.

Done

---

TODO

• We will probably need more fallbacks that return nothing when called on an object that doesn't support the trait. I've just added these to the old GeoInterface for bbox and crs so we can call it just in case, as e.g. Shapefiles have bbox precalculated for everything, but other gemoetries may not.
• We should make sure that coordnames and x/yconstant-propagates and findfist isn't a runtime lookup.

And

• Implement this in other packages (at least the most used)
• Think about automatic converts between geometries based on GeoInterface
• Do we need a native WKT/WKB implementation?

[rafaqz]

Looking pretty solid to me. I probably have to see it in PRs to give more extensive feedback.

[rafaqz]

I just hit a findfirst error here - it doesn't work on Tuple.

const default_coord_names = (:X, :Y, :Z, :M)

x(::AbstractPointTrait, geom) = getcoord(geom, findfirst(coordnames(geom), :X))
y(::AbstractPointTrait, geom) = getcoord(geom, findfirst(coordnames(geom), :Y))
z(::AbstractPointTrait, geom) = getcoord(geom, findfirst(coordnames(geom), :Z))
m(::AbstractPointTrait, geom) = getcoord(geom, findfirst(coordnames(geom), :M))

And default_coord_names is a tuple ;)

[evetion]

I just hit a findfirst error here - it doesn't work on Tuple.

My bad, I've fixed it and added a test for it. Will add some more tests on the defaults, and enable coverage to prevent this.