-
Notifications
You must be signed in to change notification settings - Fork 0
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 CRSmode so we can dispatch on the type of crs. #38
base: master
Are you sure you want to change the base?
Conversation
struct CRS <: FormatMode end | ||
Base.show(io::IO, ::MIME"text/plain", ::Type{CRS}) = print(io, "CRS mode") | ||
struct CRS{T<:CRSMode} <: FormatMode end | ||
CRS() = CRS{UnknownCRS}() | ||
Base.show(io::IO, ::MIME"text/plain", ::Type{CRS{T}}) where {T} = print(io, "$T mode") |
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.
AFAICT, this won't work for the CoordinateReferenceSystemFormat types?
Maybe we need either a wrapper as indicated in the issue, or an extra type param in the type definition for those
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.
Yeah, I guess they will all need a type parameter.
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.
Hmm @asinghvi17 your comment in the issue makes a good point that wrapping the other way is simpler, instead of adding heaps of type parameters to every object here we just compose the objects here as-is with other singletons.
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.
Can you show a MWE for a wrapper? I'd like to adjust the existing types to use them as they're implemented across the ecosystem.
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 was thinking of something on the lines of:
struct CRS{T, CRSTrait} <: CoordinateReferenceSystemFormat
val::T
CRS(x::CoordinateReferenceSystemFormat, ::CRSTrait) where CRSTrait = CRS{typeof(x), CRSTrait}(x)
end
CRS(x::CoordinateReferenceSystemFormat) = CRS(x, GI.crstrait(x))
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.
then, the GI.crs
of a geometry might be CRS{EPSG, GeographicCRSTrait}(EPSG((4326,)))
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.
That MWE probably won't work as is, but you get the idea
I'm not totally clear on where these will be set to something besides Is the idea that somewhere some package calls Like is this for Rasters/ArchGDAL/GeoDataFrames to set? Shapefile/GeoJSON etc wont have that option. GeometryOps.jl only has Proj in an extension so it cant be default behaviour to add this. Probably also Proj and ArchGDAL should set this too whenever they return a |
I guess we could also use the GeoInterface crstrait stuff directly here? |
Sadly, GeoInterface depends on GFT, so we can't, but they are indeed 1 on 1 linked. Producers should set these formats directly, so AG, Proj, Rasters and GeoDataFrames. GeometryOps and GeoMakie could consume and choose correct algorithms. |
Partially solves #30.
As a follow up to JuliaGeo/GeoInterface.jl#140. With this PR we can implement crstrait for GFT types, which can then be set and used across the ecosystem when reading crs information.