sis-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From desruisse...@apache.org
Subject [sis] branch geoapi-4.0 updated: More robust concatenation of transforms when some steps may be dropping or adding a dimension.
Date Thu, 07 May 2020 15:21:07 GMT
This is an automated email from the ASF dual-hosted git repository.

desruisseaux pushed a commit to branch geoapi-4.0
in repository https://gitbox.apache.org/repos/asf/sis.git


The following commit(s) were added to refs/heads/geoapi-4.0 by this push:
     new 7498ef3  More robust concatenation of transforms when some steps may be dropping
or adding a dimension.
7498ef3 is described below

commit 7498ef3fc02859c7cb2bc23c114554dac9829416
Author: Martin Desruisseaux <martin.desruisseaux@geomatys.com>
AuthorDate: Thu May 7 17:19:50 2020 +0200

    More robust concatenation of transforms when some steps may be dropping or adding a dimension.
---
 .../operation/transform/ConcatenatedTransform.java | 66 +++++++++++++++++++---
 .../operation/transform/IdentityTransform.java     | 10 +++-
 .../transform/ConcatenatedTransformTest.java       | 43 +++++++++++++-
 3 files changed, 110 insertions(+), 9 deletions(-)

diff --git a/core/sis-referencing/src/main/java/org/apache/sis/referencing/operation/transform/ConcatenatedTransform.java
b/core/sis-referencing/src/main/java/org/apache/sis/referencing/operation/transform/ConcatenatedTransform.java
index 987429e..801919d 100644
--- a/core/sis-referencing/src/main/java/org/apache/sis/referencing/operation/transform/ConcatenatedTransform.java
+++ b/core/sis-referencing/src/main/java/org/apache/sis/referencing/operation/transform/ConcatenatedTransform.java
@@ -37,6 +37,7 @@ import org.apache.sis.internal.referencing.provider.GeocentricAffine;
 import org.apache.sis.internal.referencing.WKTKeywords;
 import org.apache.sis.internal.referencing.Resources;
 import org.apache.sis.internal.system.Semaphores;
+import org.apache.sis.internal.system.Loggers;
 import org.apache.sis.internal.util.Strings;
 import org.apache.sis.util.Classes;
 import org.apache.sis.util.ComparisonMode;
@@ -44,6 +45,7 @@ import org.apache.sis.util.Utilities;
 import org.apache.sis.io.wkt.Convention;
 import org.apache.sis.io.wkt.Formatter;
 import org.apache.sis.io.wkt.FormattableObject;
+import org.apache.sis.util.logging.Logging;
 import org.apache.sis.util.resources.Errors;
 
 
@@ -56,7 +58,7 @@ import org.apache.sis.util.resources.Errors;
  * <p>Concatenated transforms are serializable if all their step transforms are serializable.</p>
  *
  * @author  Martin Desruisseaux (IRD, Geomatys)
- * @version 1.0
+ * @version 1.1
  *
  * @see org.opengis.referencing.operation.MathTransformFactory#createConcatenatedTransform(MathTransform,
MathTransform)
  *
@@ -182,7 +184,7 @@ class ConcatenatedTransform extends AbstractMathTransform implements Serializabl
 
     /**
      * Tries to returns an optimized concatenation, for example by merging two affine transforms
-     * into a single one. If no optimized cases has been found, returns {@code null}. In
the later
+     * into a single one. If no optimized case has been found, returns {@code null}. In the
later
      * case, the caller will need to create a more heavy {@link ConcatenatedTransform} instance.
      *
      * @param  factory  the factory which is (indirectly) invoking this method, or {@code
null} if none.
@@ -240,6 +242,53 @@ class ConcatenatedTransform extends AbstractMathTransform implements
Serializabl
          * If both transforms use matrix, then we can create
          * a single transform using the concatenated matrix.
          */
+        final MathTransform concatenated = multiply(tr1, tr2, factory);
+        if (concatenated instanceof AbstractLinearTransform) {
+            /*
+             * Following code computes the inverse of `concatenated` transform. In principle
this is
+             * not necessary because `MathTransform.inverse()` would do that computation
itself when
+             * first needed. However if the matrices are not square (e.g. if a transform
is dropping
+             * a dimension) some information may be lost. By computing inverse transform
immediately
+             * as the concatenation of the inverse of individual transforms, we use information
that
+             * would otherwise be lost (e.g. the inverse of the transform dropping a dimension
may be
+             * a transform setting that dimension to a constant value, often zero). Consequently
the
+             * inverse transform here may have real values for coefficients that `MathTransform.inverse()`
+             * would have set to NaN, or may succeed while `MathTransform.inverse()` would
have throw
+             * an exception. Even with square matrices, computing the inverse transform now
may avoid
+             * some rounding errors.
+             */
+            final AbstractLinearTransform impl = (AbstractLinearTransform) concatenated;
+            if (impl.inverse == null) try {
+                final MathTransform inverse = multiply(tr2.inverse(), tr1.inverse(), factory);
+                if (inverse != null) {
+                    if (inverse.isIdentity()) {
+                        return inverse;
+                    }
+                    if (inverse instanceof LinearTransform) {
+                        impl.inverse = (LinearTransform) inverse;
+                    }
+                }
+            } catch (NoninvertibleTransformException e) {
+                /*
+                 * Ignore. The `concatenated.inverse()` method will try to compute the inverse
itself,
+                 * possible at the cost of more NaN values than what above code would have
produced.
+                 */
+                Logging.recoverableException(Logging.getLogger(Loggers.COORDINATE_OPERATION),
+                        ConcatenatedTransform.class, "create", e);
+            }
+        }
+        return concatenated;
+    }
+
+    /**
+     * Returns a transform resulting from the multiplication of the matrices of given transforms.
+     * If the given transforms does not provide matrix, then this method returns {@code null}.
+     *
+     * @param  factory  the factory which is (indirectly) invoking this method, or {@code
null} if none.
+     */
+    private static MathTransform multiply(final MathTransform tr1, final MathTransform tr2,
+            final MathTransformFactory factory) throws FactoryException
+    {
         final Matrix matrix1 = MathTransforms.getMatrix(tr1);
         if (matrix1 != null) {
             final Matrix matrix2 = MathTransforms.getMatrix(tr2);
@@ -251,9 +300,9 @@ class ConcatenatedTransform extends AbstractMathTransform implements Serializabl
                 /*
                  * NOTE: It is quite tempting to "fix rounding errors" in the matrix before
to create the transform.
                  * But this is often wrong for datum shift transformations (Molodensky and
the like) since the datum
-                 * shifts are very small. The shift may be the order of magnitude of the
tolerance threshold. Intead,
+                 * shifts are very small. The shift may be the order of magnitude of the
tolerance threshold. Instead,
                  * Apache SIS performs matrix operations using double-double arithmetic in
the hope to get exact
-                 * results at the 'double' accuracy, which avoid the need for a tolerance
threshold.
+                 * results at the `double` accuracy, which avoid the need for a tolerance
threshold.
                  */
                 if (factory != null) {
                     return factory.createAffineTransform(matrix);
@@ -355,10 +404,13 @@ class ConcatenatedTransform extends AbstractMathTransform implements
Serializabl
     }
 
     /**
-     * Returns all concatenated transforms, modified with the pre- and post-processing required
for WKT formating.
+     * Returns all concatenated transforms, modified with the pre- and post-processing required
for WKT formatting.
      * More specifically, if there is any Apache SIS implementation of Map Projection in
the chain, then the
      * (<var>normalize</var>, <var>normalized projection</var>, <var>denormalize</var>)
tuples are replaced by single
      * (<var>projection</var>) elements, which does not need to be instances
of {@link MathTransform}.
+     *
+     * <p>This method is used only for producing human-readable parameter values.
+     * It is not used for coordinate operations or construction of operation chains.</p>
      */
     private List<Object> getPseudoSteps() {
         final List<Object> transforms = new ArrayList<>();
@@ -821,8 +873,8 @@ class ConcatenatedTransform extends AbstractMathTransform implements Serializabl
 
     /**
      * Concatenates or pre-concatenates in an optimized way this transform with the given
transform, if possible.
-     * This method try to delegate the concatenation to {@link #transform1} or {@link #transform2}.
Assuming that
-     * transforms are associative, this is equivalent to trying the following arrangements:
+     * This method tries to delegate the concatenation to {@link #transform1} or {@link #transform2}.
+     * Assuming that transforms are associative, this is equivalent to trying the following
arrangements:
      *
      * {@preformat text
      *   Instead of : other → tr1 → tr2
diff --git a/core/sis-referencing/src/main/java/org/apache/sis/referencing/operation/transform/IdentityTransform.java
b/core/sis-referencing/src/main/java/org/apache/sis/referencing/operation/transform/IdentityTransform.java
index 6cce32a..0663f4c 100644
--- a/core/sis-referencing/src/main/java/org/apache/sis/referencing/operation/transform/IdentityTransform.java
+++ b/core/sis-referencing/src/main/java/org/apache/sis/referencing/operation/transform/IdentityTransform.java
@@ -63,6 +63,13 @@ final class IdentityTransform extends AbstractLinearTransform {
      */
     private IdentityTransform(final int dimension) {
         this.dimension = dimension;
+        /*
+         * This value is not used by `inverse()` method, but we set it anyway because
+         * `ConcatenatedTransform.createOptimized(…)` checks if the value is non-null.
+         * A null value would cause unwanted computation of inverse transform(because
+         * `ConcatenatedTransform` computes it itself.
+         */
+        inverse = this;
     }
 
     /**
@@ -224,10 +231,11 @@ final class IdentityTransform extends AbstractLinearTransform {
     }
 
     /**
-     * Returns the inverse transform of this object, which is this transform itself
+     * Returns the inverse transform of this object, which is this transform itself.
      */
     @Override
     public LinearTransform inverse() {
+        // Avoid unnecessary access to `inverse` volatile field.
         return this;
     }
 
diff --git a/core/sis-referencing/src/test/java/org/apache/sis/referencing/operation/transform/ConcatenatedTransformTest.java
b/core/sis-referencing/src/test/java/org/apache/sis/referencing/operation/transform/ConcatenatedTransformTest.java
index bf29c78..aebf03f 100644
--- a/core/sis-referencing/src/test/java/org/apache/sis/referencing/operation/transform/ConcatenatedTransformTest.java
+++ b/core/sis-referencing/src/test/java/org/apache/sis/referencing/operation/transform/ConcatenatedTransformTest.java
@@ -19,11 +19,13 @@ package org.apache.sis.referencing.operation.transform;
 import org.opengis.util.FactoryException;
 import org.opengis.referencing.operation.MathTransform;
 import org.opengis.referencing.operation.TransformException;
+import org.opengis.referencing.operation.NoninvertibleTransformException;
 import org.apache.sis.internal.referencing.j2d.AffineTransform2D;
 import org.apache.sis.referencing.operation.matrix.Matrices;
 import org.apache.sis.referencing.operation.matrix.Matrix4;
 import org.apache.sis.test.DependsOn;
 import org.junit.Test;
+import org.opengis.test.Assert;
 
 import static org.opengis.test.Assert.*;
 
@@ -32,7 +34,7 @@ import static org.opengis.test.Assert.*;
  * Tests the {@link ConcatenatedTransform} class.
  *
  * @author  Martin Desruisseaux (Geomatys)
- * @version 1.0
+ * @version 1.1
  * @since   0.5
  * @module
  */
@@ -147,4 +149,43 @@ public final strictfp class ConcatenatedTransformTest extends MathTransformTestC
         assertEquals("Source dimensions", 3, transform.getSourceDimensions());
         assertEquals("Target dimensions", 4, transform.getTargetDimensions());
     }
+
+    /**
+     * Tests concatenation of transforms built from non-square matrices. The transforms are
invertible
+     * when taken separately, but the transform resulting from concatenation can not be inverted
unless
+     * {@link ConcatenatedTransform#createOptimized(MathTransform, MathTransform, MathTransformFactory)}
+     * prepares in advance the inverse transform using the inverse of original transforms.
+     *
+     * @throws NoninvertibleTransformException if a transform can not be inverted.
+     */
+    @Test
+    public void testNonSquares() throws NoninvertibleTransformException {
+        final LinearTransform tr1 = MathTransforms.scale(8, 6, 0.5);
+        final LinearTransform tr2 = MathTransforms.linear(Matrices.create(4, 3, new double[]
{
+            2, 0, 0,        // Scale first dimension.
+            0, 3, 0,        // Scale second dimension.
+            0, 0, 5,        // Set third dimension to a constant.
+            0, 0, 1}));     // Usual row in affine transforms.
+        /*
+         * Request for a transform going from 3D points to 2D points.
+         * Dropping a dimension is not a problem.
+         */
+        final MathTransform c = MathTransforms.concatenate(tr1, tr2.inverse());
+        Assert.assertMatrixEquals("Forward", Matrices.create(3, 4, new double[] {
+            4, 0, 0, 0,     // scale = 8/2
+            0, 2, 0, 0,     // scale = 6/3
+            0, 0, 0, 1}), MathTransforms.getMatrix(c), 0);
+        /*
+         * Following test is the interesting part. By inverting the transform, we ask for
a conversion
+         * from 2D points to 3D points. Without contextual information we would not know
which value to
+         * put in the third dimension (that value would fallback on NaN). But with the knowledge
that
+         * this concatenation was built from a transform which was putting value 5 in third
dimension,
+         * we can complete the matrix as below with value 10 in third dimension.
+         */
+        Assert.assertMatrixEquals("Inverse", Matrices.create(4, 3, new double[] {
+            0.25, 0,    0,
+            0,    0.5,  0,
+            0,    0,   10,   // Having value 10 instead of NaN is the main purpose of this
test.
+            0,    0,    1}), MathTransforms.getMatrix(c.inverse()), 0);
+    }
 }


Mime
View raw message