-
Notifications
You must be signed in to change notification settings - Fork 20
add coordtype
#167
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
add coordtype
#167
Conversation
| coordtype(::Point{<:Any,<:Any,<:AbstractArray{T}}) where T = T | ||
| coordtype(::Point{<:Any,<:Any,<:NTuple{<:Any,T}}) where T = T | ||
| coordtype(::Point{<:Any,<:Any,<:NamedTuple{<:Any,<:NTuple{<:Any,T}}}) where T = T |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we remove the Point from here, so that the dispatch with parent(p) for Point feeds into these?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that will hit the fallback method in fallbacks.jl. It just get the X coord type
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we also add some @test_inferred? Or is it too early for that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, good idea
|
GeoInterface implementations: We have recently added a Packages to update
The way you implement this is that you define the two-argument GeoInterface.coordtype(::GeoInterface.FeatureCollectionTrait, featurecollection::YourAbstractFeatureCollection) = ...
GeoInterface.coordtype(::GeoInterface.FeatureTrait, feature::YourAbstractFeature) = ...
GeoInterface.coordtype(::GeoInterface.AbstractGeometryTrait, geom::YourAbstractGeometrySupertype) = ...Only implement the feature and feature collection trait methods if those are types in the repo. For example, WellKnownGeometry, LibGEOS and GeometryBasics have no feature collection or feature types. Clone each repo and make the change, then make a PR. For some you may have to fork. |
|
I'm fine with this, but might be good to give a concrete example of what this improves. Do we also want to give some hints using coordtype for let's say GI.coordinates? That's the most type unstable thing I can think of in here. |
|
Yeah, we could use it for (Answered the rest in the issue thread) |
|
https://gdal.org/en/stable/doxygen/classOGRPoint.html#a5170ea70ce7458059e4395f852fce687 indicates that ArchGDAL must also set this to Float64, so it's just Julia types that need to have the type statically encoded somehow. |
|
Is there anything required or can we just merge? |
|
Merged. Id like to wait for extent computation and then get a minor release out. |
Closes #128
Currently on a GI.MultiPolygon this takes 4ns. Its not a compile time operation. We could put this in the type parameters of the wrapper geometries at construction so its instant.
A lot of packages will be able to set this absolutely to a single type, e.g. Shapefile.jl, GeoJSON.jl and LibGEOS.jl can return
Float64.