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

Horner and helmert #456

Merged
merged 27 commits into from
Dec 15, 2016
Merged

Horner and helmert #456

merged 27 commits into from
Dec 15, 2016

Conversation

busstoptaktik
Copy link
Member

This PR introduces the Horner polynomial evaluator and the Helmert 7 parameter shift as new proj entries. It also cleans up the code of the pj_fwd/inv/fwd3d/inv3d functions, making them compatible with proj entries which do not necessarily accept angular input and provide linear output.

Introducing the Horner polynomial evaluator also introduces the need for very long +init:tag arguments (a n'th order 2D polynomium has (n+1)(n+2)/2 coefficients, and n is typically in the range 5-10, i.e. up to around 60 coefficients for each polynomium, and there are 4 polynomia in a complete back/forward transformation set).

Hence, in this PR, along with the first part of the Horner code, the code for reading +init files has been modified in a (for all practical purposes) backwards compatible way, by making it possible to introduce line continuations by escaping line breaks, i.e. preceding them with a backslash.

An escaped line break works (as it would in TeX), by skipping all following whitespace, including interspersed #-comments. This simple extension makes it possible to create very long initialization elements without losing track of the structure (cf. s45b.pol and pj_init_test.c in the examples-directory for a demo).

The s45b.pol file was created by hand-editing the output of the software doing the original constrained adjustment for the polynomial coefficients. The simple adding of the “skip following whitespace and comments” feature has made it possible to retain almost all metadata from the source material.

This is considered very important, since 1) For the lack of a prior common file format for geodetic polynomial coefficients, there is a good chance that this will become THE standard, at least for the time being, and 2) Without the metadata represented, it will be very hard for a human to debug code involving a slightly misrepresented polynomium.

Due to the current architecture of the pj_init.c code (mostly around the fill_buffer() function), it is next to impossible to implement the line continuation functionality in full generality. Hence, it has been necessary to limit this format extension to files smaller than 64 kB.

Thomas Knudsen added 8 commits November 20, 2016 06:17
Introducing the Horner polynomial evaluator also introduces the need for
very long +init:tag arguments (a n'th order 2D polynomium has
(n+1)(n+2)/2 coefficients, and n is typically in the range 5-10, i.e. up
to around 60 coefficients for each polynomium, and there are 4 polynomia
in a complete back/forward transformation set).

Hence, in this commit, along with the first part of the Horner code, the
code for reading +init files has been modified in a (for all practical
purposes) backwards compatible way, by making it possible to introduce
line continuations by escaping line breaks, i.e. preceding them with a
backslash.

An escaped line break works (as it would in TeX), by skipping all
following whitespace, including interspersed #-comments. This simple
extension makes it possible to create very long initialization elements
without losing track of the structure (cf. s45b.pol and pj_init_test.c
in the examples-directory for a demo).

The s45b.pol file was created by hand-editing the output of the software
doing the original constrained adjustment for the polynomial
coefficients. The simple adding of the “skip following whitespace and
comments” feature has made it possible to retain almost all metadata
from the source material.

This is considered very important, since 1) For the lack of a prior
common file format for geodetic polynomial coefficients, there is a good
chance that this will become THE standard, at least for the time being,
and 2) Without the metadata represented, it will be very hard for a
human to debug code involving a slightly misrepresented polynomium.

Due to the current architecture of the pj_init.c code (mostly around the
fill_buffer() function), it is next to impossible to implement the line
continuation functionality in full generality. Hence, it has been
necessary to limit this format extension to files smaller than 64 kB.
Getting ready for some consultancy with Karsten Engsager, who has
maintained the original code for more than 2 decades.
@hobu
Copy link
Contributor

hobu commented Nov 29, 2016

It also cleans up the code

It is probably a better practice to not mix development efforts into pull requests. Doing so makes it hard to track changes and report activities come release time.

@busstoptaktik
Copy link
Member Author

@hobu: Sorry, that was probably not clearly stated by me. What I meant to say was that, as part of this, I had to add the extension to the fwd/inv functions, since helmert and horner return the same type of coordinates that they accept. While adding that, the code, incidentally, also ended up somewhat cleaner.

Thomas Knudsen added 2 commits November 30, 2016 10:25
The line continuation format extension was misimplemented: The comment
skipping logic was placed on the wrong side of a while loop
@busstoptaktik busstoptaktik self-assigned this Dec 2, 2016
@busstoptaktik busstoptaktik added this to the 4.9.4 milestone Dec 2, 2016
Thomas Knudsen added 6 commits December 4, 2016 09:23
The first HEALpix test case in nad/testvarious is clearly intended to
invoke the spherical form of HEALpix.

It does, however, specify the spheroid using the +a=1 size parameter,
without specifying any shape parameter.

But since +no_defs is not specified either, a shape parameter is picked
up from the nad/proj_def.dat file (where ellps=WGS84 is given in the
<general> section).

For reasons that I fail to comprehend, it appears that this has not
happened before I updated the pj_init code to support projection
pipelines.

I do, however, believe that the present behaviour is the correct one,
and rather than retrohacking the pj_init code, to (incorrectly, I
believe) reproduce the prior behaviour, I have corrected the test case
invocation in nad/testvarious to specify the spheroid using the +R=1
size parameter (which was already used in the following test case).
<horner.h> appears to work with  cmake build, but not with autotools
Copy link
Member

@kbevers kbevers left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a bit overwhelming to go through. There is a lot of stuff going on, but it does seem rather reasonable to me.

Is there a reason for separating the Horner-stuff in horner.hand horner.c? It goes a bit against the grain compared to other "projections".

Any idea what it is that trips up the tests?

Tiny test of an evolving new API, demonstrating examples of init-file
handling.

NOTE!!!! set PROJ_LIB=. before running this
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this really needed? Further down you state the direct path to the init-file with +init=./s45b.pol:s45b

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tests were tripped due to the scaling functions in pj_inv*/pj_fwd*. This is repaired for now, but a better solution is possible for "non-classic" style functions (cf comments in a recent commit).

Regarding pj_init_test.c in general: It has been removed from the repository now, and may be replaced by a pj_pipeline_test.c demo, once the final parts (molodensky transformations and tran.c) are done.

double dist;


/* (55,12,100) -> (x,y,z), ed50, fra KMStrans2 */
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

English, please :-) fra -> from. There's a few more below.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removed (ibid.)

@@ -174,8 +174,8 @@ int pj_calcofi_selftest (void) {
double tolerance_lp = 1e-10;
double tolerance_xy = 1e-7;

char e_args[] = {"+proj=calcofi +ellps=GRS80 +lat_1=0.5 +lat_2=2"};
char s_args[] = {"+proj=calcofi +a=6400000 +lat_1=0.5 +lat_2=2"};
char e_args[] = {"+proj=calcofi +ellps=GRS80 +lat_1=0.5 +lat_2=2 +no_defs"};
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are no default presets for calcofi so +no_defs should really not be necessary here. At least when testing under known conditions such as on travis and appveyor. On the other hand it is probably a good idea to add +no_defs to all test transformations in (the unlikely, I guess) case someone uses a custom proj_def.dat file.

Nothing to do with this PR, just thinkinig out loud....

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The pj_inv/fwd scaling "bug" was detected due to this projection stomping on setup data.

Thomas Knudsen added 4 commits December 13, 2016 17:41
The "return const err object" idiom (i.e.  const <type> err =
{HUGE_VAL,...}; ...  if (bad) return err) is problematic to implement
due to MSVC's misimplementation of HUGE_VAL as a non-const.

Hence, we need to run-time initialize these. In the pj_inv functions,
this was mistakenly done to the wrong object.

For pj_fwdobs/invobs and the remaining part of the obs-based API, this
is now worked around by providing functions returning a run time
HUGE_VAL initialized PJ_OBS or PJ_COO resp.

Obnoxious, but given MSVC's market penetration there is really not much
else we can do.
As per a review comment by Kristian Evers, @kbevers
Initializing after use in pj_fwd/pj_fwd3d. Forgotten #include horner.h
in PJ_horner.c
@busstoptaktik busstoptaktik merged commit 44c611c into OSGeo:master Dec 15, 2016
@busstoptaktik busstoptaktik deleted the Horner-and-Helmert branch December 15, 2016 15:47
@kbevers kbevers modified the milestones: 5.0.0-b, 5.0.0 Feb 1, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants