JuliaGeo / Proj.jl

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

make PJ_COORD a FieldVector #50

Closed visr closed 3 years ago

visr commented 3 years ago

@c42f this is what you had in mind in your comment here https://github.com/JuliaGeo/Proj4.jl/pull/44#pullrequestreview-429425928 right?

https://proj.org/development/reference/datatypes.html#complex-coordinate-types

This will allow access to the PJ_COORD coordinates by either index or x,y,z,t index. That means it matches the first two here:

typedef union {
    double v[4];
    PJ_XYZT xyzt;
    PJ_UVWT uvwt;
    PJ_LPZT lpzt;
    PJ_GEOD geod;
    PJ_OPK opk;
    PJ_ENU enu;
    PJ_XYZ  xyz;
    PJ_UVW  uvw;
    PJ_LPZ  lpz;
    PJ_XY   xy;
    PJ_UV   uv;
    PJ_LP   lp;
} PJ_COORD ;

This feels slightly odd since we are making the PJ_XYZT special. But if it is really a PJ_UVWT folks can just use p[1] instead of p.x, which would be misnaming the element. Though judging by the argument names in https://proj.org/development/reference/functions.html#c.proj_coord it seems PROJ itself sometimes does the same.

visr commented 3 years ago

Perhaps we should just rename it to Coord, since it will be part of a high level interface as well.

And I should remove all the PJ_XYZT and co, that are not used anywhere.

c42f commented 3 years ago

I think this was what I had in mind, yes. Though https://github.com/JuliaGeo/Proj4.jl/pull/44#pullrequestreview-429425928 was some time ago now!

Perhaps we should just rename it to Coord since it will be part of a high level interface as well.

Yes, Proj4.Coord seems like an excellent name.

This feels slightly odd since we are making the PJ_XYZT special

You can allow access to the other fields by implementing getproperty. It's a little finicky, but IIRC you should be able to get zero overhead with code like

function Base.getproperty(c::Coord, f::Symbol)
    f === :x ? getfield(c, :x) :
    f === :y ? getfield(c, :y) :
    f === :z ? getfield(c, :z) :
    f === :t ? getfield(c, :t) :
    f === :u ? getfield(c, :x) :
    f === :v ? getfield(c, :y) :
    f === :w ? getfield(c, :z) :
    # etc ...
end
visr commented 3 years ago

You can allow access to the other fields by implementing getproperty. It's a little finicky, but IIRC you should be able to get zero overhead with code like

Thanks for this. After thinking about it, I'd say lets go for the current definition, we can add bells and whistles later if needed. I documented that both indexing and field access can be used: https://github.com/JuliaGeo/Proj4.jl/pull/50/commits/affd59e5547da5d90349a84f2510bdd9d3fe32d4#diff-a74c7a30fe2180fccaac7d6a5d8d48b1e2f4325f236df56a99ae9faa2291aecdR1-R10

This should be good to go as-is.

c42f commented 3 years ago

Sounds fine to me :+1: