convert any geometries to gdal types in geometry methods #366
Conversation
rafaqz
force-pushed
the
convert_in_methods
branch
from
b113963
to
d860b69
Compare
rafaqz
force-pushed
the
convert_in_methods
branch
from
d860b69
to
84f3955
Compare
There was a problem hiding this comment.
Thank you for the PR description -- it answered my questions. There doesn't seem to be a way to look at code coverage delta, but LGTM apart from maintaining coverage there. I'll also ask for approval from @visr @evetion since they have context from JuliaGeo/GeoInterface.jl#78
| end | ||
|
|
||
| @noinline _not_a_geom_error() = | ||
| throw(ArgumentError("Ojbect of type $(typeof(geom)) is not a GeoInterface compatible geometry")) |
| # TODO Add `length` method to match GeoInterface naming conventions? | ||
| # this will mean using `Base.length` everywhere |
There was a problem hiding this comment.
Should we turn this into a github issue?
| # @test AG.asbinary( | ||
| # AG.fromWKT("POLYGON((0 0, 10 0, 10 10, 0 10, 0 0))"), | ||
| # )[1:10] == | ||
| # UInt8[0x01, 0x03, 0x00, 0x00, 0x00, 0x01, 0x00, 0x00, 0x00, 0x05] | ||
|
|
||
| # @test AG.astext(AG.fromWKT("POLYGON((0 0, 10 0, 10 10, 0 10, 0 0))")) == | ||
| # "POLYGON ((0 0,10 0,10 10,0 10,0 0))" |
There was a problem hiding this comment.
Out of curiosity, why are these commented out?
There was a problem hiding this comment.
Oh because they dont exist, but maybe should to match geointerface
There was a problem hiding this comment.
I'll add a comment and a github issue, it's basically the same as for length
There was a problem hiding this comment.
I just found the report of the coverage -- https://app.codecov.io/gh/yeesian/ArchGDAL.jl/pull/366 and it seems a good number of methods still needs coverage
|
Yeah im slowly working through them |
Add methods to operations like
disjoint,crossesetc to accept arbitrary GeoInterface.jl compatible geometries and convert them to ArchGDAL geometries with the sharedto_gdalmethod andgeointerface_geomtype.The returned values are always still
ArchGDALgeometries.We don't use
convert(ArchGDAL, geom)here as it has a small overhead to call a pacage method, which is unnecessary to pay locally - especially for points of anything there are a lot of.Needs tests, and JuliaGeo/GeoInterface.jl#78