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

Add GFT.val definitions for the CRS defined here #40

Merged
merged 2 commits into from
May 16, 2024
Merged

Add GFT.val definitions for the CRS defined here #40

merged 2 commits into from
May 16, 2024

Conversation

asinghvi17
Copy link
Member

No description provided.

@asinghvi17 asinghvi17 changed the title Update MapTiles.jl Add GFT.val definitions for the CRS defined here May 12, 2024
@asinghvi17 asinghvi17 marked this pull request as ready for review May 12, 2024 15:40
@asinghvi17 asinghvi17 requested a review from rafaqz May 12, 2024 15:40
@rafaqz
Copy link
Member

rafaqz commented May 13, 2024

Why are these not just const aliases for EPSG(x)?

@asinghvi17
Copy link
Member Author

You can't dispatch on the int within an EPSG I think

@rafaqz
Copy link
Member

rafaqz commented May 13, 2024

Right. I guess I'm a little worried that convert to other projections wont work anywhere in the ecosystem currently.

I think we need to define convert methods on these objects and other GeoFormat wrappers and then make them call convert again on the EPSG or just return the EPSG object if that's the target.

(maybe thats a separate PR though)

@asinghvi17
Copy link
Member Author

asinghvi17 commented May 13, 2024

All (Proj-compatible) projections can be said to have a "canonical form" which is basically what Proj accepts, so in that case maybe we add a GFT method canonical or something, that guarantees it returns a Proj-compatible String?

This way people can define their own CRS structs for dispatch purposes, while still letting them be proj-compatible.

By Proj-compatible, I mean expressible in WKT etc, so Albers USA would not qualify.

@rafaqz
Copy link
Member

rafaqz commented May 13, 2024

Ok maybe I need to explain a little more...

Most of the purpose of GFT as a package is that various geospatial data formats only allow using a specific kind of string/number for CRS (Proj/wellknowntext/epsg/etc) and by wrapping a known string or number in a wrapper type every package knows exactly what the thing is without parsing it, and is e.g. allowed to write the contents to a file.

We need a convert method so that packages can convert to the format that they specifically need.

My issue is not about Proj compatability at all because proj has automatic detection. Its about all the other packages that don't depend on a binary like proj/gdal being able to either know a CRS is already in the right format for them, or have an external package they don't depend on do the conversion by pirating convert for GFT and telling users to load it by default. By just calling required_crs = convert(RequiredGTFtype, crs_we_were_given) and ignoring the details.

Currently ArchGDAL is the anointed external package that does that because historically most packages that needed this depended on ArchGDAL already, but it could also just be Proj. The other packages don't even know about this, they just call convert.

But Base.convert(::Type{<:GeoFormat}, ...) is the key method in GFT, and it not working makes something being <: GFT not worth very much.

(and I also get that I wrote a bunch of the code here and never fixed this 😅 eek)

Also another thought, with such specific types here, we could just include the convert methods manually and not need Proj at all for proj/WKT convert. But this is another PR, it only made sense to bring up given the possibility to just use EPSG as a reason why thats a good idea...

@asinghvi17
Copy link
Member Author

Makes sense - and that should definitely go in the GFT docs! I don't mind implementing converts for these particular CRSs here, but we should then probably abstract this out to a separate package at some stage...

@rafaqz
Copy link
Member

rafaqz commented May 13, 2024

and that should definitely go in the GFT docs!

Absolutely, I haven't been the best communicator with all these things

but we should then probably abstract this out to a separate package at some stage

It could also just go in GFT.jl under an e.g. ExplicitCoordinateReferenceSystem abstract type or something? and we could have a list of common things like these two? I don't know, I never really understood how this should work, maybe also why the documentation is bad - the idea was pretty vague.

@rafaqz
Copy link
Member

rafaqz commented May 13, 2024

What would to_conventional do?

@asinghvi17
Copy link
Member Author

asinghvi17 commented May 13, 2024

A translation to any native GFT type so that convert can just call that, otherwise you'd have to reimplement all convert functions per type (they may be one liners, but even so)

@rafaqz
Copy link
Member

rafaqz commented May 13, 2024

Arent they all one-liners too ? ;)

@asinghvi17
Copy link
Member Author

Well you'd have to define a convert for each separate type (EPSG, ProjJSON, ProjString, EsriWKT, KML, ...) but with a conventional/canonical form, you don't have to do that

@rafaqz
Copy link
Member

rafaqz commented May 13, 2024

Sorry I don't get it... my reason to define the exact conversion is to skip needing Proj/ArchGDAL for any conversion at all, and just make them instant. The canonical version still needs those for most conversions.

(Well, we wouldn't necessarily specify well-known binary, in that case and similar, using a canonical epsg would help)

@asinghvi17
Copy link
Member Author

I'm not saying that we should define only the canonical conversion, just that this fallback should exist (I probably wouldn't end up implementing an explicit KML method, for example, and it could be useful for that to have an automatic fallback).

@rafaqz
Copy link
Member

rafaqz commented May 16, 2024

Makes sense

@rafaqz rafaqz merged commit 7f1bea1 into master May 16, 2024
2 of 4 checks passed
@rafaqz rafaqz deleted the as/crs branch May 16, 2024 19:18
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