JuliaGeo / Proj.jl

Julia wrapper for the PROJ cartographic projections library
MIT License
48 stars 9 forks source link

Return the same type which was provided when transforming #65

Closed asinghvi17 closed 2 years ago

asinghvi17 commented 2 years ago

Basically, instead of returning an NTuple, this returns the same type which was provided when calling t::Transformation.

asinghvi17 commented 2 years ago

The specific behaviour which I think caught me a bit off guard was that when I used to pass an array in, I would get an array out; however, this is no longer the case. I think that was from the StaticArrays interface, which makes sense.

To me, the best way to move forward is probably to explicitly define transformations on Points in GeoMakie.

asinghvi17 commented 2 years ago

I have defined the following functions in GeoMakie:

function (transformation::Proj.Transformation)(coord::Point{N, T}) where {N, T <: Real}
    @assert 2 ≤ N ≤ 4
    return Point{N, T}(transformation(coord.data))
end

function (transformation::Proj.Transformation)(coord::Vec{N, T}) where {N, T <: Real}
    @assert 2 ≤ N ≤ 4
    return Point{N, T}(transformation(coord.data))
end

which seem to solve the issues I had previously.

visr commented 2 years ago

Thanks! This PR still needs doc and test updates, but if this works well and is still fast I'm in favor.

The specific behaviour which I think caught me a bit off guard was that when I used to pass an array in, I would get an array out; however, this is no longer the case. I think that was from the StaticArrays interface, which makes sense.

I'd like to wrap proj_trans_array and proj_trans_generic to offer an alternative (optionally inplace) interface for transforming many points. Not sure yet how much of a performance boost that gives over broadcasting over proj_trans like we do now. Also not sure what the interface should look like.

To me, the best way to move forward is probably to explicitly define transformations on Points in GeoMakie.

To be clear, with this PR those function definitions are not needed anymore right?

asinghvi17 commented 2 years ago

oops...this PR doesn't really work yet, I will have to look some more into it. I have a final coming up in two days but after that can definitely polish up a good system. The main issue is that we don't know the form of the most efficient constructor for a given type - e.g., Point2 can take a tuple or just 2 args, as can any StaticArray-derived type, but regular Arrays cannot.

visr commented 2 years ago

The main issue is that we don't know the form of the most efficient constructor for a given type - e.g., Point2 can take a tuple or just 2 args, as can any StaticArray-derived type, but regular Arrays cannot.

Ah yes I think I ran into this as well. Unless there is a general solution perhaps we should just go ahead with returning a tuple of Float64 as in #64. It's easy enough to convert that tuple back into say a Point2f if that is what you want to continue working with. It's better to replace this:

https://github.com/JuliaPlots/GeoMakie.jl/blob/ea3e71baa108a90f924172bf59de3519380bd769/src/utils.jl#L72-L82

With just adding a convertion on the tuple that comes back. And if you own the types, you can always overload of course.

asinghvi17 commented 2 years ago

Makes sense - it will definitely cause less problems. I can pretty easily generalize this in GeoMakie.