search

yeesian / ArchGDAL.jl

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

Implement new GeoInterface traits #290

Closed evetion closed 2 years ago

evetion commented 2 years ago

GeoInterface 1.0 makes use of Traits, this is a PR to update our pre-1.0 GeoInterface implementation.

Note that I had to add some methods to align with GeoInterface:

I've also added dependencies on GeoInterfaceRecipes, a plot package based on GeoInterface, which previously was integrated into GeoInterface and on Extents, a package defining bounding boxes as NamedTuples.

I also note that much in ArchGDAL doesn't support Z en M coordinates properly, for which I will create a separate issue. In short, many of our creation methods don't result in the correct type (just a Point instead of Point25D etc.). This makes it hard to correctly implement getcoord for the m dimension, as it could be both the third or fourth dimension, depending on the type.

evetion commented 2 years ago
visr commented 2 years ago

Benchmark CI doesn't seem to update to latest GeoInterface (still using v0.5.7?)

That'd be because of Shapefile.jl

https://github.com/yeesian/ArchGDAL.jl/blob/e401e0079d97dac9d4755a4bc9dce345a9c5bf81/benchmark/Project.toml#L6

evetion commented 2 years ago

@rafaqz can you take a look here at the convert options? I keep running into ambiguous things, as our suggetion for implementing convert overrides everything, including existing methods such as those for GeoFormatTypes.

  Expression: sprint(print, convert(AG.IGeometry, json)) == "Geometry: POINT (100 70)"
  MethodError: convert(::Type{ArchGDAL.IGeometry}, ::GeoFormatTypes.GeoJSON{String}) is ambiguous. Candidates:
    convert(::Type{T}, source::X) where {T<:ArchGDAL.AbstractGeometry, X<:GeoFormatTypes.GeoJSON} in ArchGDAL at /Users/evetion/code/ArchGDAL.jl/src/convert.jl:48
    convert(::Type{T}, geom::X) where {T<:ArchGDAL.IGeometry, X} in ArchGDAL at /Users/evetion/code/ArchGDAL.jl/src/geointerface.jl:262
  Possible fix, define
    convert(::Type{T}, ::X) where {T<:ArchGDAL.IGeometry, X<:GeoFormatTypes.GeoJSON}
rafaqz commented 2 years ago

We need to dispatch both on ArchGDAL.AbstractGeometry. That will work. Both first arguments need to be equally specific

evetion commented 2 years ago

We need to dispatch both on ArchGDAL.AbstractGeometry. That will work. Both first arguments need to be equally specific.

Well that's what I did first, but it segfaults preparedgeometry tests(!). Something to investigate, but otherwise, I'm leaning towards the idea that only IGeometries should be created from conversion, while all geometries can function as a source. But maybe better as an issue than to keep changing more and more in this PR.

rafaqz commented 2 years ago

Both can be on IGeometry as well I guess? I cant remember the distinctions honestly.

yeesian commented 2 years ago

I'm leaning towards the idea that only IGeometries should be created from conversion, while all geometries can function as a source

Yeah I think this might be a viable approach (or at least it was how I was mentally thinking about things without properly having articulated them anywhere)

evetion commented 2 years ago

Benchmark should be able to run when https://github.com/JuliaGeo/Shapefile.jl/pull/72 is merged and released.

evetion commented 2 years ago

Ok, I can't merge without review approval from someone 👀. I think it's done, coverage has been increased as asked by @yeesian. I've fixed the comments by @rafaqz and @visr. The benchmark suite seems to require a released ArchGDAL version to test against, so it still fails.