sis-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From desruisse...@apache.org
Subject [sis] 03/03: Fix a wrong transform concatenation occurring when matrix coefficients are very close to zero. Bug identified by Johann Sorel.
Date Wed, 07 Jul 2021 09:53:03 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

commit 0df0bc4dbd79bf84adbcdcce5ba05aa84ee86fb1
Author: Martin Desruisseaux <martin.desruisseaux@geomatys.com>
AuthorDate: Wed Jul 7 11:14:49 2021 +0200

    Fix a wrong transform concatenation occurring when matrix coefficients are very close
to zero.
    Bug identified by Johann Sorel.
---
 .../sis/referencing/operation/matrix/Matrices.java |  5 +++-
 .../referencing/operation/matrix/MatricesTest.java | 17 ++++++++++++
 .../transform/ConcatenatedTransformTest.java       | 30 ++++++++++++++++++++--
 3 files changed, 49 insertions(+), 3 deletions(-)

diff --git a/core/sis-referencing/src/main/java/org/apache/sis/referencing/operation/matrix/Matrices.java
b/core/sis-referencing/src/main/java/org/apache/sis/referencing/operation/matrix/Matrices.java
index 80d194a..a7beb6c 100644
--- a/core/sis-referencing/src/main/java/org/apache/sis/referencing/operation/matrix/Matrices.java
+++ b/core/sis-referencing/src/main/java/org/apache/sis/referencing/operation/matrix/Matrices.java
@@ -1141,7 +1141,10 @@ public final class Matrices extends Static {
                     final double v2 = m2.getElement(j, i);
                     double tolerance = epsilon;
                     if (relative) {
-                        tolerance *= Math.max(Math.abs(v1), Math.abs(v2));
+                        final double f = Math.max(Math.abs(v1), Math.abs(v2));
+                        if (f <= Double.MAX_VALUE) {
+                            tolerance *= f;
+                        }
                     }
                     if (!(Math.abs(v1 - v2) <= tolerance)) {
                         if (Numerics.equals(v1, v2)) {
diff --git a/core/sis-referencing/src/test/java/org/apache/sis/referencing/operation/matrix/MatricesTest.java
b/core/sis-referencing/src/test/java/org/apache/sis/referencing/operation/matrix/MatricesTest.java
index 7c75c00..ace10ee 100644
--- a/core/sis-referencing/src/test/java/org/apache/sis/referencing/operation/matrix/MatricesTest.java
+++ b/core/sis-referencing/src/test/java/org/apache/sis/referencing/operation/matrix/MatricesTest.java
@@ -40,6 +40,7 @@ import static org.opengis.referencing.cs.AxisDirection.*;
  * Tests the {@link Matrices} implementation.
  *
  * @author  Martin Desruisseaux (Geomatys)
+ * @author  Johann Sorel (Geomatys)
  * @version 1.1
  * @since   0.4
  * @module
@@ -455,6 +456,22 @@ public final strictfp class MatricesTest extends TestCase {
     }
 
     /**
+     * Tests {@link Matrices#equals(Matrix, Matrix, double, boolean)}.
+     */
+    @Test
+    public void testEquals() {
+        Matrix2 m1 = new Matrix2(-1.001, 0, 0, 1);
+        Matrix2 m2 = new Matrix2(-1,     0, 0, 1);
+        assertTrue(Matrices.equals(m1, m2, 0.002, true));
+        /*
+         * An infinite value with a relative tolerance threshold causes an infinite threshold,
+         * which is undesirable. Verify that the comparison code handle is robust to infinities.
+         */
+        m1 = new Matrix2(Double.POSITIVE_INFINITY, 0, 0, 1);
+        assertFalse(Matrices.equals(m1, m2, 0.002, true));
+    }
+
+    /**
      * Tests {@link Matrices#copy(Matrix)}
      */
     @Test
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 5a49e40..51030a0 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
@@ -17,11 +17,13 @@
 package org.apache.sis.referencing.operation.transform;
 
 import org.opengis.util.FactoryException;
+import org.opengis.referencing.operation.Matrix;
 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.Matrix2;
 import org.apache.sis.referencing.operation.matrix.Matrix4;
 import org.apache.sis.test.DependsOn;
 import org.junit.Test;
@@ -34,6 +36,7 @@ import static org.opengis.test.Assert.*;
  * Tests the {@link ConcatenatedTransform} class.
  *
  * @author  Martin Desruisseaux (Geomatys)
+ * @author  Johann Sorel (Geomatys)
  * @version 1.1
  * @since   0.5
  * @module
@@ -41,6 +44,11 @@ import static org.opengis.test.Assert.*;
 @DependsOn(ProjectiveTransformTest.class)
 public final strictfp class ConcatenatedTransformTest extends MathTransformTestCase {
     /**
+     * Tolerance factor for strict equalities.
+     */
+    private static final double STRICT = 0;
+
+    /**
      * Tests the concatenation of two affine transforms that can be represented
      * as a {@link ConcatenatedTransformDirect2D}.
      *
@@ -174,7 +182,7 @@ public final strictfp class ConcatenatedTransformTest extends MathTransformTestC
         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);
+            0, 0, 0, 1}), MathTransforms.getMatrix(c), STRICT);
         /*
          * 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
@@ -186,6 +194,24 @@ public final strictfp class ConcatenatedTransformTest extends MathTransformTestC
             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);
+            0,    0,    1}), MathTransforms.getMatrix(c.inverse()), STRICT);
+    }
+
+    /**
+     * Tests a concatenation between transforms having (indirectly) infinite coefficients.
+     * This test uses a transform with a coefficient close enough to zero for causing the
+     * inverse matrix to have infinite values. If the coefficient was strictly zero, a
+     * {@link org.apache.sis.referencing.operation.matrix.NoninvertibleMatrixException}
+     * would have been thrown. But with non-zero coefficient small enough, the exception
+     * is not thrown and infinite values may confuse {@link ConcatenatedTransform} if not
+     * properly handled.
+     */
+    @Test
+    public void testInfinities() {
+        final MathTransform tr1 = MathTransforms.linear(new Matrix2(4.9E-324, -5387, 0, 1));
+        final MathTransform tr2 = MathTransforms.linear(new Matrix2(-1, 0, 0, 1));
+        final MathTransform c   = MathTransforms.concatenate(tr1, tr2);
+        final Matrix m          = ((LinearTransform) c).getMatrix();
+        Assert.assertMatrixEquals("Concatenate", new Matrix2(-4.9E-324, 5387, 0, 1), m, STRICT);
     }
 }

Mime
View raw message