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

make PJ_COORD a FieldVector #50

Merged
merged 4 commits into from
Jan 16, 2021
Merged

make PJ_COORD a FieldVector #50

merged 4 commits into from
Jan 16, 2021

Conversation

visr
Copy link
Member

@visr visr commented Jan 13, 2021

@c42f this is what you had in mind in your comment here #44 (review) 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
Copy link
Member Author

visr commented Jan 13, 2021

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
Copy link
Member

c42f commented Jan 14, 2021

I think this was what I had in mind, yes. Though #44 (review) 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

Since they are disconnected from the rest of the code, and not directly usable.
and leave out the unusable structs PJ_XY from proj_common.jl
@visr
Copy link
Member Author

visr commented Jan 14, 2021

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: affd59e#diff-a74c7a30fe2180fccaac7d6a5d8d48b1e2f4325f236df56a99ae9faa2291aecdR1-R10

This should be good to go as-is.

@visr visr merged commit b315f16 into master Jan 16, 2021
@visr visr deleted the fieldvector branch January 16, 2021 15:11
visr added a commit that referenced this pull request Jan 17, 2021
This backtracks #50 mostly.

It doesn't quite feel right to name the fields x and y even though the axis order is up to the CRS by default.

In [the PROJ docs](https://proj.org/development/reference/functions.html?highlight=coordinate%20components#c.proj_trans_generic) they had to include this note for instance:

> Even though he coordinate components are named x, y, z and t, axis ordering of the to and from CRS is respected. Transformations exhibit the same behavior as if they were gathered in a PJ_COORD struct.
@c42f
Copy link
Member

c42f commented Jan 17, 2021

Sounds fine to me 👍

visr added a commit that referenced this pull request Feb 16, 2021
This backtracks #50 mostly.

It doesn't quite feel right to name the fields x and y even though the axis order is up to the CRS by default.

In [the PROJ docs](https://proj.org/development/reference/functions.html?highlight=coordinate%20components#c.proj_trans_generic) they had to include this note for instance:

> Even though he coordinate components are named x, y, z and t, axis ordering of the to and from CRS is respected. Transformations exhibit the same behavior as if they were gathered in a PJ_COORD struct.

(cherry picked from commit 1d0c03e)
visr added a commit that referenced this pull request Feb 18, 2021
This backtracks #50 mostly.

It doesn't quite feel right to name the fields x and y even though the axis order is up to the CRS by default.

In [the PROJ docs](https://proj.org/development/reference/functions.html?highlight=coordinate%20components#c.proj_trans_generic) they had to include this note for instance:

> Even though he coordinate components are named x, y, z and t, axis ordering of the to and from CRS is respected. Transformations exhibit the same behavior as if they were gathered in a PJ_COORD struct.

(cherry picked from commit 1d0c03e)
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