Plumbing for pipelines #453

busstoptaktik · 2016-11-18T11:27:28Z

This is a much improved reimplementation of the ideas behind PR #388 "Transformation Pipelines".

This PR implements the library code only. An upcomming PR will reintroduce the TRAN command line interface, and a handful of elementary transformations typically used in geodetic transformation pipelines.

The pipeline interface is now internally based on the pj_obs_api, which simplifies the implementation significantly. This is the first mock up - it compiles fine, but is currently untested

The pipeline code is now based on the PJ_OBS api (although you can still invoke a pipeline through pj_fwd/pj_inv and their 3D brethren). This has made it possible to eliminate scores of funky casts and convoluted workarounds. The code is now way more straightforward and mostly conforming with common C idioms.. Also, the proj.h / obs_api interface to the logging system has been streamlined through the introduction of the pj_log_error, pj_log_debug, and pj_log_trace functions.

First proj.h style interface to Charles Karney's geodesics code: pj_lp_dist. Also, renamed pj_apply -> pj_trans

Second eccentricity, second and third flattening etc.

... and add self test code for PJ_pipeline

kbevers

I actually haven't got very many comments to the code. It seems quite well thought-out to me. Good job, Thomas.

The pipeline concept is very powerful, but also rather complicated compared to what has been possible in PROJ.4 up until now. This needs to be documented thoroughly. I suggest a new section on the webpage, "Coordinate transformation" where the concept of pipelines can be introduced. It will also be a nice place to document the geodetic transformations that are coming from you soon. Additionally the new proj.h/pj_obs_api.c API should be documented in the "Development" section. This doesn't have to be a part of this PR, but should be written at least before the next release and preferably a lot sooner so first-movers has a chance to try things out.

kbevers · 2016-11-18T14:57:31Z

src/PJ_pipeline.c

+
+************************************************************************
+
+**The Problem**


You're not using these three headings. Might as well remove them.

Added the description, the headings were hinting at instead :-)

Works for me too. Most of the text can be used as a general introduction in the docs as well.

kbevers · 2016-11-18T16:32:36Z

src/PJ_pipeline.c

+        if (0==strcmp ("step", argv[i])) {
+            if (-1==i_pipeline) {
+                pj_log_error (P, "Pipeline: +step before +proj=pipeline");
+                return pipeline_freeup (P, 51);


You are setting the error number på 51 here. It seems like an arbitrary number to me. How about defining a constant? Like EPIPELINE or something in the PJ_ namespace. The same error could be used below (instead of 50 and 52). I think having one error-type for everything pipeline related is good enough.

This part is definitely a bug - thanks for spotting it. The pj_strerrno values must be negative. They are, however, mostly used by proj, geod, and cs2cs, so I'm not sure it's worth the effort to introduce symbolic constants. I'll consider whether it makes sense to open an enhancement proposal to implement this.

Corrected by introducing element 50 in pj_strerrno, and replacing all error message numbers in PJ_pipeline to -50.

They are, however, mostly used by proj, geod, and cs2cs, so I'm not sure it's worth the effort to introduce symbolic constants. I'll consider whether it makes sense to open an enhancement proposal to implement this.

Me neither. Usually though, I have no idea what a given error number means and I'll have to look it up. Could just as well be solved by comments in the code. Labelling the error numbers with good names fixes that and makes the code more readable. But that is outside the scope of this PR i think :)

@kbevers

(In response to a review by @kbevers)

QuLogic

Alas, this was merged before I finished reading it, but I think the parsing could perhaps have been simplified a bit.

QuLogic · 2016-11-20T04:48:02Z

src/PJ_pipeline.c

+    last_step  = P->opaque->steps + 1;
+    incr  = 1;
+
+    for (i = first_step; i != last_step; i += incr) {


Does the increment actually change? Why not just i++?

Again for (mostly) historical reasons - the code came from a function that combined the forward and inverse implementations.

Will be cleaned up in connection with the 0-offset stuff

QuLogic · 2016-11-20T04:50:34Z

src/PJ_pipeline.c

+static PJ_OBS pipeline_forward_obs (PJ_OBS point, PJ *P) {
+    int i, first_step, last_step, incr;
+
+    first_step = 1;


Is this not the conventional 0-based access for a reason?

It's actually mostly for historical reasons (in the previous version of the pipeline stuff, P[0] was the pipeline driver itself, and a sentinel was needed in the far end of the pipeline, because P was an array of PJs, not of PJ *.

I intend to clean this up at a later date, but I need to go on with some material actually using the pipeline functionality in order to stress test the implementation

QuLogic · 2016-11-20T04:59:19Z

src/PJ_pipeline.c

+        return 0;
+
+    argc = argc_params (P->params);
+    argv = argv_params (P->params);


Pass argc to argv_params to avoid an extra iteration through P->params? Alternatively, argv = argv_params(P->params, &argc);?

Thanks - implementing this improvement as part of comming PR

QuLogic · 2016-11-20T05:04:46Z

src/PJ_pipeline.c

+
+    /* The elements of current_argv are not used - we just use argv_params */
+    /* as allocator for a "large enough" container needed later            */
+    current_argv = argv_params (P->params);


That's two extra iterations through P->params when you already have argc and can just calloc it, no?

Thanks again - have implemented it for next PR

QuLogic · 2016-11-20T05:07:20Z

src/PJ_pipeline.c

+        return pipeline_freeup (P, ENOMEM);
+
+    /* Do some syntactic sanity checking */
+    for (i = 0;  i < argc;  i++) {


The calculation of argc or argv could have been folded into here.

I agree it could be done, but I'm not sure how to do it without reducing code clarity.

I think it should be possible in just two iterations to do everything; I can open a PR for it.

Yes, in two iterations. I think I misunderstood: Thought you wanted to fold everything info the syntax check loop - which can be done, but not very elegantly. And as this part is not at all a speed concern, I preferred to just implement the first two suggestions, eliminating two trips through the stack, and Still keeping the code clean.

No, definitely not just one loop, as that would require assuming some maximum size of each step or some messy reallocing.

Two loops should work though, first loop counts nstep and maximum args-per-step, and second loop populates argv for parsing each step's arguments.

See https://github.com/busstoptaktik/proj.4/blob/Horner-and-Helmert/src/PJ_pipeline.c#L359-L388 for the current implementation, which I think captures most of your intentions

busstoptaktik · 2016-11-20T10:18:09Z

Sorry for fat-fingering a merge (due to a momentarily sluggish mouse response), while you were reviewing. I appreciate your comments and will implement most of them in comming PRs

Thomas Knudsen added 5 commits November 14, 2016 16:26

re-enter pipeline

d56a940

The pipeline interface is now internally based on the pj_obs_api, which simplifies the implementation significantly. This is the first mock up - it compiles fine, but is currently untested

Geodesics + minor changes

dcaf85b

First proj.h style interface to Charles Karney's geodesics code: pj_lp_dist. Also, renamed pj_apply -> pj_trans

Extended Ellipsoidal Parameters

35df792

Second eccentricity, second and third flattening etc.

Rename pj_debug_set -> pj_log_level

4711daf

... and add self test code for PJ_pipeline

busstoptaktik mentioned this pull request Nov 18, 2016

Transformation pipelines #388

Closed

Thomas Knudsen added 2 commits November 18, 2016 12:56

Clean up missing pj_apply->pj_trans

6249b47

Clean up missing pj_obs_dist_2d rename

8cdc347

busstoptaktik self-assigned this Nov 18, 2016

busstoptaktik modified the milestones: 4.10.0, 4.9.4 Nov 18, 2016

busstoptaktik added C: Core enhancement labels Nov 18, 2016

kbevers requested changes Nov 18, 2016

View reviewed changes

busstoptaktik mentioned this pull request Nov 19, 2016

Documentation for pipelines #454

Closed

pj_strerrno bug fixed. Some doc/comments added

f518cc6

(In response to a review by @kbevers)

kbevers approved these changes Nov 19, 2016

View reviewed changes

busstoptaktik merged commit fab2015 into OSGeo:master Nov 20, 2016

busstoptaktik deleted the plumbing-for-pipelines branch November 20, 2016 05:04

QuLogic reviewed Nov 20, 2016

View reviewed changes

kbevers modified the milestones: 5.0.0-b, 5.0.0 Feb 1, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Plumbing for pipelines #453

Plumbing for pipelines #453

busstoptaktik commented Nov 18, 2016

kbevers left a comment

kbevers Nov 18, 2016

busstoptaktik Nov 19, 2016

kbevers Nov 19, 2016

kbevers Nov 18, 2016

busstoptaktik Nov 19, 2016

busstoptaktik Nov 19, 2016

kbevers Nov 19, 2016

QuLogic left a comment

QuLogic Nov 20, 2016

busstoptaktik Nov 20, 2016

QuLogic Nov 20, 2016

busstoptaktik Nov 20, 2016

QuLogic Nov 20, 2016

busstoptaktik Nov 20, 2016

QuLogic Nov 20, 2016

busstoptaktik Nov 20, 2016

QuLogic Nov 20, 2016

busstoptaktik Nov 20, 2016

QuLogic Nov 20, 2016

busstoptaktik Nov 20, 2016

QuLogic Nov 20, 2016

busstoptaktik Nov 21, 2016 •

edited

busstoptaktik commented Nov 20, 2016


		************************************************************************

		The Problem

Plumbing for pipelines #453

Plumbing for pipelines #453

Conversation

busstoptaktik commented Nov 18, 2016

kbevers left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

QuLogic left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

busstoptaktik Nov 21, 2016 • edited

Choose a reason for hiding this comment

busstoptaktik commented Nov 20, 2016

busstoptaktik Nov 21, 2016 •

edited