Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement new GeoInterface traits #290

Merged
merged 19 commits into from
May 25, 2022
Merged

Implement new GeoInterface traits #290

merged 19 commits into from
May 25, 2022

Conversation

evetion
Copy link
Collaborator

@evetion evetion commented Apr 10, 2022

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:

  • Added getm method, previously not wrapped
  • Added getgeom for LineStrings. Technically OGR doesn't expose this (and wants us to use getpoint), but our ngeom does have a switch for Lines.
  • Added is3d and ismeasured to be used when getting the the third or fourth coordinate
  • Added keys and values to a Feature

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.

@visr visr marked this pull request as draft April 23, 2022 19:38
@evetion
Copy link
Collaborator Author

evetion commented May 17, 2022

  • Benchmark CI doesn't seem to update to latest GeoInterface (still using v0.5.7?)
  • Plots recipes are required for the documentation to work @rafaqz

@visr
Copy link
Collaborator

visr commented May 17, 2022

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

That'd be because of Shapefile.jl

Shapefile = "8e980c4a-a4fe-5da2-b3a7-4b4b0353a2f4"

src/geointerface.jl Outdated Show resolved Hide resolved
@evetion
Copy link
Collaborator Author

evetion commented May 20, 2022

@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
Copy link
Collaborator

rafaqz commented May 21, 2022

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

@evetion
Copy link
Collaborator Author

evetion commented May 21, 2022

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
Copy link
Collaborator

rafaqz commented May 21, 2022

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

@evetion evetion marked this pull request as ready for review May 21, 2022 16:11
src/geointerface.jl Outdated Show resolved Hide resolved
@yeesian
Copy link
Owner

yeesian commented May 21, 2022

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
Copy link
Collaborator Author

evetion commented May 21, 2022

Benchmark should be able to run when JuliaGeo/Shapefile.jl#72 is merged and released.

yeesian
yeesian previously approved these changes May 22, 2022
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.

Overall LGTM, thank you so much!

Might be good to update the top level comment of this PR with the changes.

src/geointerface.jl Show resolved Hide resolved
@yeesian yeesian linked an issue May 22, 2022 that may be closed by this pull request
Copy link
Collaborator

@rafaqz rafaqz left a comment

Choose a reason for hiding this comment

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

Few minor comments, otherwise LGTM

src/geointerface.jl Outdated Show resolved Hide resolved
src/geointerface.jl Outdated Show resolved Hide resolved
@evetion
Copy link
Collaborator Author

evetion commented May 24, 2022

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.

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.

LGTM, thank you so much for seeing this through!

@evetion evetion merged commit 6f6868e into master May 25, 2022
@evetion evetion deleted the feat/geointerface-traits branch May 25, 2022 06:30
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.

is3d and ismeasured are missing
4 participants