-
Notifications
You must be signed in to change notification settings - Fork 768
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
Generic coordinates #431
Generic coordinates #431
Conversation
Inroducing the OBSERVATION data type as the basis for generic geodetic transformations. Also introducing the elements of a new minimalistic API focused on generic geodetic transformations. This API is documented in the new proj.h header, and is orthogonal (non-intrusive) wrt. the existing API from proj_api.h Finally added a large amount of comments to the somewhat intractable projects.h, and extended the PJ object with a number of additional ellipsoidal parameters of general geodetic usefulness.
Can you remove the extra merges? |
@QuLogic Don't worry about those, they can be squashed before merging. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, Thomas. I like this new no-nonsense API!
A few general comments:
- There's mixed use of tabs and spaces in proj.h and projects.h. I have tried to point them out with line comments.
- A documentation page with a few examples and explanations would be really good to have before merging this.
- I like the restructured projects.h. Much easier to navigate in now.
#endif | ||
|
||
/* Avoid preprocessor renaming and implicit type-punning: Use a union to make it explicit */ | ||
union PJ_TRIPLET { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mixed whitespace in this block
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done :-)
|
||
************************************************************************************** | ||
|
||
For projection xxx, these are pointers to functions in the corresponding |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mixed whitespace in this comment section
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done :-)
a name which is currently hidden behind the magic curtain of the PROJECTION | ||
macro. | ||
|
||
As the PROJ.4 de-macroization project expands its coverage, this will change, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest leaving this paragraph out. The odds of this message surviving the intendd de-macroization are quite high and will most likely lead to confusion for future readers of the code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done :-)
|
||
int datum_type; /* PJD_UNKNOWN/3PARAM/7PARAM/GRIDSHIFT/WGS84 */ | ||
double datum_params[7]; /* Parameters for 3PARAM and 7PARAM */ | ||
struct _pj_gi **gridlist; /* Description needed */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"Description needed" is not a very useful comment. It would be good to have those descriptions made before merging. Or at least a ticket so it is not forgotten once this PR is merged.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done :-)
#ifdef PJ_LIB__ | ||
/* repetitive projection code */ | ||
/* repetitive projection code (most of them eliminated now) */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Elimaniting these macros is a very good idea. Could you create a ticket that describes this effort? It will be easier to keep track of that way, and there's a bigger change someone will pick it up and do the work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done :-)
|
||
|
||
/* Direction: "+1" forward, "-1" reverse, 0: roundtrip precision check (not fully done) */ | ||
OBSERVATION pj_apply (PJ *P, int direction, OBSERVATION obs) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I bet I'll have to look up the meaning og +1, -1 and 0 everytime I use it. Declaring constants like PJ_FORWARD, PJ_REVERSE and PJ_ROUNDTRIP (or something similar) will make calls to pj_apply() self-documenting.
Challenge: Come up with a self-documenting function name that isn't already use :) pj_transform would be a perfect name for this, but is already used... Anyone?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done :-)
…sponse to code review by @kbevers. The PJ elements fwdobs and invobs extend fwd3d and inv3d in a homologous way to how fwd3d and inv3d extend fwd and inv.
Thanks, @kbevers - I have implemented the requested improvements, and added a few missing details logically belonging to this PR. |
@QuLogic - does this update look sensible to you (given that commits are squashed on merge) |
@@ -72,7 +72,9 @@ libproj_la_SOURCES = \ | |||
pj_apply_gridshift.c pj_datums.c pj_datum_set.c pj_transform.c \ | |||
geocent.c geocent.h pj_utils.c pj_gridinfo.c pj_gridlist.c \ | |||
jniproj.c pj_mutex.c pj_initcache.c pj_apply_vgridshift.c geodesic.c \ | |||
pj_strtod.c | |||
pj_strtod.c \ | |||
\ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why the extra line?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because a new block starts here: During the reimplementation of pipelines additional files will be added to this block
g.anc.opk.k = k; | ||
g.id = id; | ||
g.flags = flags; | ||
return g; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's a tab here instead of the spaces above. Not sure what the convention is for proj.4, but I guess it should be consistent.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for spotting this - apparently a last minute touch up after having done "convert tabs to spaces".
switch (direction) { | ||
case 1: | ||
obs.coo.xyz = pj_fwd3d (obs.coo.lpz, P); | ||
return obs; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These lines are a very complicated mix of tabs and spaces.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes - that's certainly an odd mix. Probably from a cut/paste operation made in hell. Cleaned up now
@@ -58,9 +58,11 @@ support = \ | |||
pj_ctx.obj pj_fileapi.obj pj_log.obj pj_apply_vgridshift.obj \ | |||
pj_strtod.obj pj_run_selftests.obj pj_generic_selftest.obj | |||
|
|||
pipeline = \ | |||
pj_observation.obj |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Blank line after (and there probably should have been one after geodesic
, I imagine.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Your imagination is right :-)
#endif /* ndef PROJECTS_H */ | ||
|
||
/* Data type for generic geodetic 3D data */ | ||
union PJ_TRIPLET; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason to declare this instead of moving the typedef
after the definition?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mostly to get a preamble style overview of "what's being defined in this file", by having declarations before the more messy definitions. During development, and the combinatorial puzzle of getting projects.h and proj.h to play nice together, the preamble sunk down below the conditional redefinitions of the proj pair/triplet types.
I have moved them back to the top, and hope you see the usefulness of the preamble style.
OBSERVATION pj_apply (PJ *P, int direction, OBSERVATION obs); | ||
|
||
/* Direction: "+" forward, "-" reverse, 0: roundtrip precision check */ | ||
enum pj_apply_direction {PJ_APPLY_FWD = -1, PJ_APPLY_ROUNDTRIP = 0, PJ_APPLY_INV = 1}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this not be used in pj_apply
's declaration (not that it makes much difference in C)? Also, I think this should go on multiple lines.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm slightly undecided on this... I introduced the enum in response to a comment by @kbevers who felt the need for a linguistic, rather than mathematical, reminder of "what is what".
My own feeling is that having the "direction" argument directly indicate the (mathematically interpreted) power of the operator (1 forward, -1 inverse) would make ample sense. And since in geodesy numerical stability is important, I would like to have an additional version of the roundtrip test, where e.g. direction=2000 would mean "deviation from input observation for a series of 2000 roundtrips".
Hence, I would prefer to keep the direction argument an int, for future use
#ifndef PROJECTS_H | ||
OBSERVATION pj_apply (PJ *P, int direction, OBSERVATION obs); | ||
|
||
/* Direction: "+" forward, "-" reverse, 0: roundtrip precision check */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Direction of what?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Direction of transformation - apply the forward (1) or inverse (-1) transformation. I have added a few explanatory comments for the pj_apply function
void (*spc)(LP, PJ *, struct FACTORS *); | ||
|
||
void (*pfree)(PJ *); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure three empty lines plus the asterisks is really necessary.
struct ARG_list { | ||
paralist *next; | ||
char used; | ||
char param[1]; /* This probably should be [0] to be fully standards compliant? */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, this is correct; [0]
would be an empty array. What the code does is allocate the size of this structure + the length of the string. The extra 1 here accounts for the NUL terminator on the string.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I understand the reason for the 1. It just that in most historical cases of the "struct hack", param[0] was used. For many years it was a matter of heated discussion whether this was standards conforming or not. C99 (or was it C11?) solved this - but not as I seemed to remember when I wrote the comment, by requiring a 0 index, but rather by requiring the declaration to be incomplete, i.e. param[].
Hence, for C99 (11?) this is definitely non-conforming (but probably portable among all relevant platforms). For C89 it has apparently not led to any problems, despite potential non-conformality.
Given this non-straightforward state of things, I chose to remove the comment in order not to add further confusion to an already muddy matter :-)
#define IS_ANAL_XL_YL 01 /* derivatives of lon analytic */ | ||
#define IS_ANAL_XP_YP 02 /* derivatives of lat analytic */ | ||
#define IS_ANAL_HK 04 /* h and k analytic */ | ||
#define IS_ANAL_HK 04 /* h and k analytic */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure this needed to move?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well - its mostly a matter of putting less important stuff lower in the file. I've responded by moving a large group of related material (defines and the definition of the projCtx_t) down into the same block of the file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't mean in the file; just the spacing left to right.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry - missed that one. Corrected now
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@QuLogic - So with this final (?) correction, can you approve the review?
Corrections of whitespace blunders minor reorganization of projects.h a number of additional comments
Thanks, @QuLogic - I have implemented (and/or elaborated on) the requested improvements, and added a small number of related corrections/improvements of consistency. |
#define TODEG(rad) ((rad)*180/M_PI) | ||
#endif | ||
#ifndef TORAD | ||
#define TORAD(deg) ((deg)*M_PI/180) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would you mind using 180.0 for these two macros? I seem to remember Visual C++ giving warnings about implicit typecasting.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for spotting this @micahcochran - not being a MSVC user myself, I am probably too unaware of MSVC peculiarities.
While writing 180.0 a place, where the type promotion rules are clear, makes the code look somewhat Fortran-ish, I find it important to get rid of that kind of noise, which may divert attention away from more important warnings.
Can you approve the change?
… by Micah Cochran Silence MSVC warnings about promotion of int to double, by changing 180 to Fortran style 180.0. As the promotion is exactly as specified in the standard it is slightly odd to warn about it. But minimizing the number of warnings makes it easier to focus on actually helpful warnings about incidentally introduced potential dangers.
I believe this PR is ready for merging now. |
int pj_show_triplet (FILE *stream, const char *banner, PJ_TRIPLET point) { | ||
int i = 0; | ||
if (banner) | ||
fprintf (stream, "%s", banner); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably i+= missing here ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks - added now
******************************************************************************/ | ||
|
||
/* Data type for generic geodetic observations */ | ||
struct OBSERVATION; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn't PJ_OBSERVATION be more consistant ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would, and I have now moved as much as possible into the PJ_ namespace. Although I find it ugly, and have numerous reservations, they are not so relevant as they used to be: Now, in most cases, union selectors (which are non-prefixed, variable-style names) will be used to address each of these types.
So I get what I want (explicit expression of "what kind of data are we working on" plus minimizing the amount of PJ_'ing). The rest of the world gets what it wants (avoid stomping on user namespaces) :-)
typedef struct PJconsts PJ; /* the PJ object herself */ | ||
|
||
/* Omega, Phi, Kappa: Rotations */ | ||
typedef struct {double o, p, k;} OPK; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given the shortness of their names and thus risk of name clash, shouldn't all those structures be PJ_ prefixed if they are intended to be in a public .h file ? (at least for the new ones)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(cf the reply to your previous comment)
}; | ||
|
||
struct OBSERVATION { | ||
PJ_SPATIOTEMPORAL coo; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps some comment to explain 'coo' and 'anc' ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have added some inline comments elaborating this
|
||
#ifndef PROJECTS_H | ||
/* Apply transformation to observation - in forward or inverse direction */ | ||
OBSERVATION pj_apply (PJ *P, int direction, OBSERVATION obs); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"enum pj_apply_direction direction" instead of "int direction" ? Or maybe create a typedef
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@QuLogic mentioned the same thing. This is my reply to him:
I'm slightly undecided on this... I introduced the enum in response to a comment by @kbevers who felt the need for a linguistic, rather than mathematical, reminder of "what is what".
My own feeling is that having the "direction" argument directly indicate the (mathematically interpreted) power of the operator (1 forward, -1 inverse) would make ample sense. And since in geodesy numerical stability is important, I would like to have an additional version of the roundtrip test, where e.g. direction=2000 would mean "deviation from input observation for a series of 2000 roundtrips".
Hence, I would prefer to keep the direction argument an int, for future use
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I never really commented on that... Why not keep the roundtrip testing in a separate function? It will probably be a lot simpler and it make it easier to create function calls that explain what they do without having to look at the API docs.
Something like
int pj_roundtrip_test(PJ *P, int direction, OBSERVATION obs, unsigned int n_roundtrips);
could be used to wrap pj_apply
and keep the test logic in a predictable place.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -0,0 +1,205 @@ | |||
/****************************************************************************** |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure how much we care about that but it would seem that /usr/include/proj.h is(was?) a SGI include file... https://www.google.fr/?gws_rd=ssl#q=%22%2Fusr%2Finclude%2Fproj.h%22
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Your google-fu is better than mine :-)
I have discussed the subject with @kbevers, and I think the common ground is that we could change the name if anyone comes up with a good one. proj4.h was mentioned...
typedef struct { double d, m, s; } DMS; | ||
|
||
/* Geoid undulation and deflections of the vertical */ | ||
typedef struct { double N, z, e; } PJ_ANC_GEOIDAL; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does ANC stand for here ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ancillary data. There's a comment about it a few lines up - I have now also added some inline comments, and changed the PJ_ANC_* names for consistency
#ifndef PROJECTS_H | ||
OPK opk; | ||
ENH enh; | ||
PJ_ANC_GEOIDAL nze; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does nze stand for ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"N" is the commonly used designation for the geoid undulation. "z" and "e" are latinizations of the greek zeta and eta, which are commonly used designations for the components of the deflection of the vertical.
#define TORAD(deg) ((deg)*M_PI/180.0) | ||
#endif | ||
|
||
extern int pj_errno; /* global error return code */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want to advertize this old/deprecated global way of getting errors in the new header file ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably a cut/paste going wrong - removed now. Thanks for spotting!
|
||
extern int pj_errno; /* global error return code */ | ||
void pj_free(PJ *P); | ||
PJ *pj_init_plus(const char *init_string); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not pj_init_plus_ctx ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As for pj_errno and pj_free, this is simply a cut/paste error. Eliminated now
More consistent use of the PJ_ namespace + additional improvements
Following a suggestion by @kbevers
Hopefully ready for merging now... Thanks you to @QuLogic @kbevers @rouault and @micahcochran for excellent and useful reviews! |
I'm 👍 to merge this too, but it should be a squash/rebase as a single commit onto master. |
Squashed and merged. Nice work, @busstoptaktik. While merging all the commits to one I've modified the initial commit message slightly:
I guess the next step is to document this new API. Here's a ticket for that: #442 |
This is the first in a series of pull requests reimplementing the functionality of PR #388 "Transformation pipelines" in a more modular way.
This first step introduces the OBSERVATION data type, and a number of additions to the PJ object, intended as the basis for extended geodetic transformation functionality.
It also introduces the elemens of a new minimalistic geodetic API, orthogonal to the original proj_api.h API