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

Use generalised convert, transform, import and similar methods instead of custom fromWKT etc methods #95

Closed
rafaqz opened this issue Nov 3, 2019 · 8 comments

Comments

@rafaqz
Copy link
Collaborator

rafaqz commented Nov 3, 2019

It would be great to do transformations between projections for points and geometries with one method, wrapping all the underlying calls. This would be fairly easy to do if crs were wrapped in types like in CoordinateReferenceSystemsBase.jl.

Something like this could just work without knowing what happens underneath because we would have all the information required to dispatch to the correct methods:

transform(sourceproj::WellKnownText, targetproj::EPSGcode, val::Geometry)

@rafaqz rafaqz changed the title Use generalised convert, transform and similar methods instead of custom fromWKT etc methods Use generalised convert, transform, import and similar methods instead of custom fromWKT etc methods Nov 3, 2019
@yeesian
Copy link
Owner

yeesian commented Nov 9, 2019

I agree; that'll be really nice! Do you have a proposal or PR in mind?

@rafaqz
Copy link
Collaborator Author

rafaqz commented Nov 10, 2019

No PR yet, But I've been working on this:
https://github.com/rafaqz/GeoFormatTypes.jl/blob/master/src/GeoFormatTypes.jl

I changed the name as some of the formats hold other kinds of data than just CRS so it should be more broadly useful. The idea is to wrap well known text, proj strings, GeoJSON etc formats at the source when they are loaded, and after than we can use dispatch to handle them throughout the ecosystem instead of having to call specific functions.

So any PR would involve registering and depending on GeoFormatTypes.jl, but its tiny and hopefully it can be more widely used. If that seems reasonable I'll put something together. We can leave the existing methods in place for now and just wrap them with the general methods.

@rafaqz
Copy link
Collaborator Author

rafaqz commented Nov 10, 2019

It would be something like this using two-step conversions where necessary:

Base.convert(::WellKnownText, source) = WellKnownText(toWKT(convert(Geometry, source)))
Base.convert(::GML, source) = GML(toGML(convert(Geometry, source))))
Base.convert(::Geometry, source::WellKnownText) = fromWKT(val(source))

Or specific conversions where gdal can do it directly (this GDAL method is made-up):

Base.convert(::WellKnownText, source::ProjString) = WellKnownText(GDAL.wkt2proj(val(source)))

Import would be like:

import!(spref::AbstractSpatialRef, source::ProjString) = begin
    result = GDAL.importfromproj4(spref.ptr, val(source))
    @ogrerr result "Failed to initialize SRS based on PROJ4 string: $(val(source))"
    spref
end

or

import!(spref::AbstractSpatialRef, source::ProjString) =  importPROJ4!(spref, val(source))

And things like this (from GeoData.jl) could be completely generalised to work with any values in any format:

reproject(coords, crs) =  begin
    AG.importWKT(GeoFormatTypes.val(crs)) do source
        AG.importEPSG(4326) do target
            AG.createcoordtrans(source, target) do transform
                transformcoord.(coords, Ref(transform))
            end
        end
    end
end

Because import would use dispatch on the source/dest types:

reproject(coords, sourceproj, destproj) =  begin
    AG.import(sourceproj) do source
        AG.import(destproj) do target
            AG.createcoordtrans(source, target) do transform
                transformcoord.(coords, Ref(transform))
            end
        end
    end
end

reproject(coords, EPSG(2025), EPSG(4326))
reproject(coords, WellKnownText("some string"), EPSG(4326))
reproject(coords, WellKnownText("some string"), ProjString("some string"))

And so on. Probably with a few issues to sort out but mostly straightforward.

@yeesian
Copy link
Owner

yeesian commented Nov 11, 2019

No PR yet, But I've been working on this:
https://github.com/rafaqz/GeoFormatTypes.jl/blob/master/src/GeoFormatTypes.jl

I changed the name as some of the formats hold other kinds of data than just CRS so it should be more broadly useful. The idea is to wrap well known text, proj strings, GeoJSON etc formats at the source when they are loaded, and after than we can use dispatch to handle them throughout the ecosystem instead of having to call specific functions.

So any PR would involve registering and depending on GeoFormatTypes.jl, but its tiny and hopefully it can be more widely used. If that seems reasonable I'll put something together. We can leave the existing methods in place for now and just wrap them with the general methods.

I like it! I'll be up for trying it out in this package and having something like that in GeoInterface.jl in the future; what do you think?

@yeesian
Copy link
Owner

yeesian commented Nov 11, 2019

btw, @visr and I have been working on a RFC for GeoInterface here: https://github.com/yeesian/GeoInterfaceRFC.jl but I haven't had the time to see it through to completion, and disseminate it more widely for feedback yet. If you like, we can maybe have it as part of the RFC too

@rafaqz
Copy link
Collaborator Author

rafaqz commented Nov 11, 2019

Ok hopefully I can get a PR together for this the next week. I'll also try to look over GeoInterfaceRFC.

I was thinking GeoInterface.jl should eventually depend on GeoFormatTypes.jl. It would be great if we could have a lot of the geospatial ecosystem depend on GeoFormatTypes.jl so they could work everywhere without specifying format, including NCDatasets.jl and other packages that don't depend on GeoInterface.jl.

@yeesian
Copy link
Owner

yeesian commented Nov 11, 2019

Ok hopefully I can get a PR together for this the next week. I'll also try to look over GeoInterfaceRFC.

I was thinking GeoInterface.jl should eventually depend on GeoFormatTypes.jl. It would be great if we could have a lot of the geospatial ecosystem depend on GeoFormatTypes.jl so they could work everywhere without specifying format, including NCDatasets.jl and other packages that don't depend on GeoInterface.jl.

Yeah sure that sounds like a better alternative

@rafaqz
Copy link
Collaborator Author

rafaqz commented Jun 3, 2020

This is done now :)

@rafaqz rafaqz closed this as completed Jun 3, 2020
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

No branches or pull requests

2 participants