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

Conversation

Projects
None yet
2 participants
@kbevers
Member

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.

kbevers added some commits Jan 11, 2018

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
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.

@kbevers kbevers requested a review from busstoptaktik Jan 11, 2018

@kbevers kbevers changed the title from Pipeline inverse to Pipeline and cct inverse fixes Jan 11, 2018

@busstoptaktik

This comment has been minimized.

Show comment
Hide comment
@busstoptaktik

busstoptaktik Jan 12, 2018

Member

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.

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

This comment has been minimized.

Show comment
Hide comment
@kbevers

kbevers Jan 12, 2018

Member

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.

Member

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

This comment has been minimized.

Show comment
Hide comment
@busstoptaktik

busstoptaktik Jan 12, 2018

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.

Member

busstoptaktik commented Jan 12, 2018

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

This comment has been minimized.

Show comment
Hide comment
@kbevers

kbevers Jan 16, 2018

Member

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?

Member

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

This comment has been minimized.

Show comment
Hide comment
@busstoptaktik

busstoptaktik Jan 16, 2018

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
Member

busstoptaktik commented Jan 16, 2018

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

This comment has been minimized.

Show comment
Hide comment
@busstoptaktik

busstoptaktik Jan 16, 2018

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.

Member

busstoptaktik commented Jan 16, 2018

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

This comment has been minimized.

Show comment
Hide comment
@kbevers

kbevers Jan 16, 2018

Member

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.

Member

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.

kbevers added some commits Jan 16, 2018

Fix "double inversions" in pipelines, require a defined forward opera…
…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

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.2%) to 74.577%
Details

@kbevers kbevers deleted the kbevers:pipeline-inverse branch Feb 1, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment