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

Add invertible Horner polynomials #3133

Merged
merged 10 commits into from
Apr 19, 2022
Merged

Conversation

georgeouzou
Copy link
Contributor

@georgeouzou georgeouzou commented Mar 23, 2022

This enables the user to create horner polynomial transformations
in the real number space without the need to supply "inv" parameters:
"+inv_origin", "+inv_u" and "+inv_v". We can get the inverse
transformation by only using the "fwd" parameters and solving
a system of equations iteratively. The system is usually solved in a
very small number of iterations.

A new optional parameter "+inv_tolerance" is added that can make the
iterative process stop earlier if a certain coordinate accuracy is
met. For example "+inv_tolerance=0.1" will make the iterative
process stop when the calculated coordinates differ less than
0.1 meters, if the units of the transformation destination is meters.

  • Tests added
  • Added clear title that can be used to generate release notes
  • Fully documented, including updating docs/source/*.rst for new API

This enables the user to create horner polynomial transformations
in the real number space without the need to supply "inv" parameters:
"+inv_origin", "+inv_u" and "+inv_v". We can get the inverse
transformation by only using the "fwd" parameters and solving
a system of equations iteratively. The system is usually solved in a
very small number of iterations.

A new optional parameter "+inv_tolerance" is added that can make the
iterative process stop earlier if a certain coordinate accuracy is
met. For example "+inv_tolerance=0.1" will make the iterative
process stop when the calculated coordinates differ less than
0.1 meters, if the units of the transformation destination is meters.
@georgeouzou
Copy link
Contributor Author

I will also try to create docs for this.

@kbevers kbevers requested a review from busstoptaktik March 23, 2022 22:46
This is due to a false positive warning.
Also add const to some variables that do not change.
In the case of inversible horner, return error values if the iterations
required for convergence are too high.
@georgeouzou
Copy link
Contributor Author

I also believe there are errors in the horner documentation

  • Input and Output types can be Geodetic or projected coordinates
  • Equation (3) seems wrong as the final coordinates are not calculated by adding the input X,Y.
  • Equation (1) seems also a little off as the Σ ui,j Ui Vj needs to be Σ ui,j Uj Vi
  • u and v in +fwd_u, +fwd_v, +inv_u, +inv_v, +origin_u, +origin_v can express northing/easting, easting/northing, latitude/longitude, longitude/latitude etc... Parameter documentation implies that u, v expresses northing/easting.

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.

I can't really comment on the maths but here's a few suggestions for improving the docs. I hope @busstoptaktik can find the time to give this a proper review at some point. He has the expertise in this part of the code base.

docs/source/operations/transformations/horner.rst Outdated Show resolved Hide resolved
docs/source/operations/transformations/horner.rst Outdated Show resolved Hide resolved
Move +inv_u, +inv_v to optional parameters in horner docs
Make some consistency changes for matrices and default values
@georgeouzou georgeouzou requested a review from kbevers March 26, 2022 13:36
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.

Doc changes looks good to me!

This enables the user to create horner polynomial transformations
in the complex number space without the need to supply "inv" parameters:
"+inv_origin" and "+inv_c".
Also added a unit test and respective docs
@georgeouzou georgeouzou changed the title Add inversible horner real polynomials Add inversible horner polynomials Mar 26, 2022
@georgeouzou
Copy link
Contributor Author

Just added complex inversion too :)

Use a similar algorithm as the normal path. This enables us to not
use std::pow at all.
Copy link
Member

@busstoptaktik busstoptaktik left a comment

Choose a reason for hiding this comment

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

Certainly a useful addition to the Horner code. See, however, my comments regarding the probable un-soundness of evaluating the polynomium by brute force, rather than Horner's scheme.

Also, IOGP Publication 373-7-2 – Geomatics Guidance Note number 7, part 2 – September 2019 has a very clear section on the criteria for polynomium reversibility. It would probably be a good thing to quote and refer extensively from/to that in the documentation updates. For example, the Geomatics Guidance Note says:

Polynomial reversibility

Approximation polynomials are in a strict mathematical sense not reversible, i.e. the same polynomial coefficients cannot be used to execute the reverse transformation

However, under certain conditions, described below, a satisfactory solution for the reverse transformation may be obtained using the forward coefficient values in a single step, rather than multiple step iteration. If such a solution is possible, in the EPSG Dataset the polynomial coordinate transformation method is classified as a reversible polynomial of degree n.

A (general) polynomial transformation is reversible only when the following conditions are met.

  1. The coordinates of source and target evaluation point are (numerically) the same.
  2. The unit of measure of the coordinate differences in source and target coordinate reference system are the same.
  3. The scaling factors applied to source and target coordinate differences are the same.
  4. The spatial variation of the differences between the coordinate reference systems around any given location is sufficiently small.

Your code solves a more general problem, and hence extends on the EPSG sense of a reversible polynomium - it probably would be a good idea to explicitly state this in the documentation.

src/transformations/horner.cpp Outdated Show resolved Hide resolved
src/transformations/horner.cpp Outdated Show resolved Hide resolved
src/transformations/horner.cpp Show resolved Hide resolved
src/transformations/horner.cpp Show resolved Hide resolved
@busstoptaktik busstoptaktik changed the title Add inversible horner polynomials Add invertible Horner polynomials Mar 31, 2022
- repurpose has_only_fwd variable to has_inv
- optimize polynomial inversion
- refactor horner evaluation code to reusable functions
@georgeouzou
Copy link
Contributor Author

@busstoptaktik i will make a separate commit for the docs.
As i understand we implemented the general case of the second bullet of Polynomial reversibility section (page 126)

  1. By applying the polynomial transformation with the same coefficients but with their signs reversed
    and then iterate to an acceptable solution, the number of iteration steps being dependent on the
    desired accuracy. (Note that only the signs of the polynomial coefficients should be reversed and not
    the coordinates of the evaluation points or the scaling factors!) The iteration procedure is usually
    described by the information source of the polynomial transformation.

So, i will update the docs, so as to make it clear this is an iterative inverse transformation and not a "reversible polynomial" as presented in the publication.

Another thing i realized when i read the section of the polynomial transformations in the publication, is that:

  • we do not support using scale factors mS and mT
  • we use evaluation points in a different way than the usage presented in the publication. Specifically we do:
    XT = ΔX,
    YT = ΔY
    instead of:
    XT = XS – XSO + XTO + ΔX,
    YT = YS – YSO + YTO + ΔY
    where ΔΧ, ΔΥ the result of the polynomium evaluation.

Cite IOGP Publication on reversible polynomials
and the difference with our general solution
@georgeouzou
Copy link
Contributor Author

georgeouzou commented Apr 16, 2022

@busstoptaktik i added the doc reference to the publication. Also i refactored the code as requested in the review.

@georgeouzou georgeouzou requested a review from kbevers April 19, 2022 13:07
@kbevers kbevers added this to the 9.1.0 milestone Apr 19, 2022
@kbevers kbevers merged commit 752e3c6 into OSGeo:master Apr 19, 2022
georgeouzou added a commit to georgeouzou/PROJ that referenced this pull request May 29, 2022
As discussed in OSGeo#3133 the existing
functions are now a bit complicated and can be refactor to simpler ones.

Checks for invalid direction are now unnecessary, as the forward and
reverse paths are only accessible through fwd4d and inv4d function
callbacks in the projection object.

Checks for a nullptr HORNER transformation object are too unnecessary as
enough checks are done in the projection object construction.
rouault pushed a commit that referenced this pull request Jun 1, 2022
As discussed in #3133 the existing
functions are now a bit complicated and can be refactor to simpler ones.

Checks for invalid direction are now unnecessary, as the forward and
reverse paths are only accessible through fwd4d and inv4d function
callbacks in the projection object.

Checks for a nullptr HORNER transformation object are too unnecessary as
enough checks are done in the projection object construction.
@busstoptaktik busstoptaktik mentioned this pull request Nov 27, 2023
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.

3 participants