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

Pipeline and cct inverse fixes #739

Merged
merged 4 commits into from Jan 17, 2018
Merged

Conversation

kbevers
Copy link
Member

@kbevers kbevers commented Jan 11, 2018

This PR makes two changes:

  1. Fail gracefully when invalid inverse operation is specified in cct.

Similar to proj and cs2cs, cct now returns immediately when trying to
do an inverse operation that is not possible, for example using
proj=urm5 which doesn't have an inverse:

	$ cct.exe -I  +proj=pipeline +step +proj=urm5 +n=0.5
	Inverse operation not available
  1. Set inv*-functions to zero on pipeline PJ's where an inverse does not exist.

Some projections do not have an inverse mapping. If such a projection is used
as a (forward) step in a pipeline we won't be able to perform an inverse
operation using the pipeline. By setting the inv, inv3d and inv4d pointers to
zero we signal to the caller that an inverse mapping is not available.

Similar to proj and cs2cs, cct now returns immediately when trying to
do an inverse operation that is not possible, for example using
proj=urm5 which doesn't have an inverse:

	$ cct.exe -I  +proj=pipeline +step +proj=urm5 +n=0.5
	Inverse operation not available
… exist.

Some projections do not have an inverse mapping. If such a projection is used
as a (forward) step in a pipeline we won't be able to perform an inverse
operation using the pipeline. By setting the inv, inv3d and inv4d pointers to
zero we signal to the caller that an inverse mapping is not available.
@kbevers kbevers changed the title Pipeline inverse Pipeline and cct inverse fixes Jan 11, 2018
@busstoptaktik
Copy link
Member

busstoptaktik commented Jan 12, 2018

Very nice that you are repairing this long standing bug, but I think, for backward compatibility you will also have to require that a forward method exists: It could be non-existing in the corner case, where at least one step is non-invertible, but have the +inv flag set, making that part of the journey a trip you can never start, but only come home from.

My imagination is not sufficient to dream up cases where this could be the intention, so I think it is safe to consider it an error: If using the new API one can always work around it by putting +inv on the pipeline itself If using the old API, this was never possible anyway.

But if I understand the PR correctly, it also needs to take into consideration the case where the pipeline itself is +inverted.

To check that all the double inversions work correctly, you should probably also add a few test cases, making sure they fail as intended.

EDIT: I see that you have filed an issue about a gie bug that prevents this. I'll look into it.

@kbevers
Copy link
Member Author

kbevers commented Jan 12, 2018

It could be non-existing in the corner case, where at least one step is non-invertible, but have the +inv flag set, making that part of the journey a trip you can never start, but only come home from.

If I understand you correctly, I take care of that particular situation in the first part of:

if ( ( Q->inverted && (Q->fwd || Q->fwd3d || Q->fwd4d) ) ||
     ( Q->inv || Q->inv3d || Q->inv4d) ) {

A comment might be warrantedt though.

EDIT: I see that you have filed an issue about a gie bug that prevents this. I'll look into it.

Yes. So that'll have to wait a bit. Also, the cct -I bit is not possible to test with gie. Not sure what the best way to test it is.

@busstoptaktik
Copy link
Member

I'm not sure: the situation I talk about is when the entire pipeline is inverted. When done at init, putting an +inv at the pipeline top level will actually lead to all steps getting inverted, and the test will work. But if done later by switching P->inverted (which happens at least one place in the source code, with a comment that we miss an inversion API call), it will not work anymore. This is why we should require the forward method to exist at the top level of the pipeline.

@kbevers
Copy link
Member Author

kbevers commented Jan 16, 2018

Still not sure I understand correctly. You want to check that a forward operation of the pipeline as a whole is possible? Or that every step-operation has a forward definition?

@busstoptaktik
Copy link
Member

Also, the cct -I bit is not possible to test with gie. Not sure what the best way to test it is.

cct inverts the operation by brute force (and will produce garbage if it is non-invertible):

/* We have no API call for inverting an operation, so we brute force it. */
if (direction==-1)
    P->inverted = !(P->inverted);
direction = 1;

Whereas gie handles the direction arg in proj_trans directly. The cct approach is probably either a blunder or a remnant from an early incarnation of the code.

When cct gets aligned with gie, the equivalent to cct -I proj=foo will be

operation proj=foo
direction inverse
accept    foobla foobla foobla
expect    bla bla bla

@busstoptaktik
Copy link
Member

You want to check that a forward operation of the pipeline as a whole is possible? Or that every step-operation has a forward definition?

Probably the problem can be solved by providing an API-entry for inverting an operation, and make that entry check whether inversion is possible.

Note that the inversion operator should not be idempotent (i.e. just setting the P->inverted flag, but be its own inverse (i.e. checking whether inversion is possible, given the current value of P->inverted, then inverting again).

In that case we will probably need two extra operators proj_fwd_exists and proj_inv_exists, takiing into account the current state of P->inverted.

I say "probably", because there is probably apps in the wild expecting that "a forward method is always available". With old API elements (pj_fwd, pj_inv) being reimplemented using new API building bricks (proj_trans), this is not always the case.

I'm not entirely sure about how this should work.

@kbevers
Copy link
Member Author

kbevers commented Jan 16, 2018

Okay, I have made a few changes. Multiple inversions are now taken care of properly and a forward path through a pipeline MUST exist. See the commit text for more details. I think this is as far as I can go without adding additional API functions that control this stuff.

…tion.

"+proj=pipeline +inv +step +urm5 +n=0.5 +inv" now works as expected,
returning the forward operation of urm5. In principle adding more +inv's
should also work, resulting in the forward operation when an even number
of +inv's are present, and the inverse when an odd number of +inv's
are present.

"+proj=pipeline +step +urm5 +n=0.5 +inv" fails at initialization since
no forward operation can be performed. This is a new requirement, but
aligns perfectly with the rest of the library since no operation without
a forward method exists.
@kbevers kbevers merged commit 33a7216 into OSGeo:master Jan 17, 2018
@kbevers kbevers deleted the pipeline-inverse branch February 1, 2018 14:53
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.

None yet

2 participants