Skip to content

Conversation

rafaqz
Copy link
Collaborator

@rafaqz rafaqz commented Jan 26, 2023

Add methods to operations like disjoint, crosses etc to accept arbitrary GeoInterface.jl compatible geometries and convert them to ArchGDAL geometries with the shared to_gdal method and geointerface_geomtype.

The returned values are always still ArchGDAL geometries.

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

@rafaqz rafaqz marked this pull request as ready for review April 22, 2023 23:23
Copy link
Owner

@yeesian yeesian left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@yeesian yeesian requested review from evetion and visr April 23, 2023 16:23
end

@noinline _not_a_geom_error() =
throw(ArgumentError("Ojbect of type $(typeof(geom)) is not a GeoInterface compatible geometry"))
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ojbect -> Object

Comment on lines +986 to +987
# TODO Add `length` method to match GeoInterface naming conventions?
# this will mean using `Base.length` everywhere
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we turn this into a github issue?

Comment on lines +338 to +344
# @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))"
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Out of curiosity, why are these commented out?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh because they dont exist, but maybe should to match geointerface

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh thanks for explaining!

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll add a comment and a github issue, it's basically the same as for length

Copy link
Owner

@yeesian yeesian left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@rafaqz
Copy link
Collaborator Author

rafaqz commented Apr 23, 2023

Yeah im slowly working through them

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants