sis-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From desruisse...@apache.org
Subject [sis] 02/02: Fix wrong adjustement of number of dimensions when the coordinate system dimensions and transform dimensions do not match.
Date Sat, 04 Jul 2020 14:38:30 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 28020712aa5def2d5d46dd4fdfc9679a4bba9412
Author: Martin Desruisseaux <martin.desruisseaux@geomatys.com>
AuthorDate: Sat Jul 4 16:37:38 2020 +0200

    Fix wrong adjustement of number of dimensions when the coordinate system dimensions and
transform dimensions do not match.
---
 .../apache/sis/internal/referencing/Resources.java |   6 +
 .../sis/internal/referencing/Resources.properties  |   1 +
 .../internal/referencing/Resources_fr.properties   |   1 +
 .../transform/DefaultMathTransformFactory.java     | 121 +++++++++++++++++----
 .../transform/DefaultMathTransformFactoryTest.java |  81 ++++++++++++--
 5 files changed, 179 insertions(+), 31 deletions(-)

diff --git a/core/sis-referencing/src/main/java/org/apache/sis/internal/referencing/Resources.java
b/core/sis-referencing/src/main/java/org/apache/sis/internal/referencing/Resources.java
index 0bc18cb..a71a1ea 100644
--- a/core/sis-referencing/src/main/java/org/apache/sis/internal/referencing/Resources.java
+++ b/core/sis-referencing/src/main/java/org/apache/sis/internal/referencing/Resources.java
@@ -71,6 +71,12 @@ public final class Resources extends IndexedResourceBundle {
         public static final short AzimuthAndDistanceNotSet = 87;
 
         /**
+         * Can not associate “{0}” transform to the given coordinate systems because
of mismatched
+         * dimensions: {1}
+         */
+        public static final short CanNotAssociateToCS_2 = 95;
+
+        /**
          * Can not create objects of type ‘{0}’ from combined URI.
          */
         public static final short CanNotCombineUriAsType_1 = 79;
diff --git a/core/sis-referencing/src/main/java/org/apache/sis/internal/referencing/Resources.properties
b/core/sis-referencing/src/main/java/org/apache/sis/internal/referencing/Resources.properties
index d84d3b3..338d505 100644
--- a/core/sis-referencing/src/main/java/org/apache/sis/internal/referencing/Resources.properties
+++ b/core/sis-referencing/src/main/java/org/apache/sis/internal/referencing/Resources.properties
@@ -45,6 +45,7 @@ NonConformCRS_3                   = The given \u201c{0}\u201d description
does n
 # Error messages (to be used in exceptions)
 #
 AzimuthAndDistanceNotSet          = The azimuth and distance have not been specified.
+CanNotAssociateToCS_2             = Can not associate \u201c{0}\u201d transform to the given
coordinate systems because of mismatched dimensions: {1}
 CanNotCombineUriAsType_1          = Can not create objects of type \u2018{0}\u2019 from combined
URI.
 CanNotComputeDerivative           = Can not compute the coordinate operation derivative.
 CanNotConcatenateTransforms_2     = Can not concatenate transforms \u201c{0}\u201d and \u201c{1}\u201d.
diff --git a/core/sis-referencing/src/main/java/org/apache/sis/internal/referencing/Resources_fr.properties
b/core/sis-referencing/src/main/java/org/apache/sis/internal/referencing/Resources_fr.properties
index 0c64847..67abae0 100644
--- a/core/sis-referencing/src/main/java/org/apache/sis/internal/referencing/Resources_fr.properties
+++ b/core/sis-referencing/src/main/java/org/apache/sis/internal/referencing/Resources_fr.properties
@@ -50,6 +50,7 @@ NonConformCRS_3                   = La description donn\u00e9e pour \u00ab\u202f
 # Error messages (to be used in exceptions)
 #
 AzimuthAndDistanceNotSet          = Le cap et la distance n\u2019ont pas \u00e9t\u00e9 d\u00e9finis.
+CanNotAssociateToCS_2             = Ne peut pas associer la transformation \u00ab\u202f{0}\u202f\u00bb
avec les syst\u00e8mes de coordonn\u00e9es parce que les dimensions ne correspondent pas:
{1}
 CanNotCombineUriAsType_1          = Ne peut pas cr\u00e9er d\u2019objets de type \u2018{0}\u2019
\u00e0 partir d\u2019un URI combin\u00e9.
 CanNotComputeDerivative           = La d\u00e9riv\u00e9 de l\u2019op\u00e9ration sur les
coordonn\u00e9es ne peut pas \u00eatre calcul\u00e9e.
 CanNotConcatenateTransforms_2     = Les transformations \u00ab\u202f{0}\u202f\u00bb et \u00ab\u202f{1}\u202f\u00bb
ne peuvent pas \u00eatre combin\u00e9es.
diff --git a/core/sis-referencing/src/main/java/org/apache/sis/referencing/operation/transform/DefaultMathTransformFactory.java
b/core/sis-referencing/src/main/java/org/apache/sis/referencing/operation/transform/DefaultMathTransformFactory.java
index f84d11d..ca1e6e6 100644
--- a/core/sis-referencing/src/main/java/org/apache/sis/referencing/operation/transform/DefaultMathTransformFactory.java
+++ b/core/sis-referencing/src/main/java/org/apache/sis/referencing/operation/transform/DefaultMathTransformFactory.java
@@ -68,6 +68,7 @@ import org.apache.sis.internal.referencing.Resources;
 import org.apache.sis.internal.system.Loggers;
 import org.apache.sis.metadata.iso.citation.Citations;
 import org.apache.sis.parameter.DefaultParameterValueGroup;
+import org.apache.sis.parameter.Parameterized;
 import org.apache.sis.parameter.Parameters;
 import org.apache.sis.referencing.IdentifiedObjects;
 import org.apache.sis.referencing.cs.AxesConvention;
@@ -424,8 +425,8 @@ public class DefaultMathTransformFactory extends AbstractFactory implements
Math
         }
         if (set == null) {
             /*
-             * Implementation note: we are better to avoid holding a lock on 'methods' and
'methodsByType'
-             * in same time because the 'methods' iterator could be a user's implementation
which callback
+             * Implementation note: we are better to avoid holding a lock on `methods` and
`methodsByType`
+             * in same time because the `methods` iterator could be a user's implementation
which callback
              * this factory.
              */
             synchronized (methods) {
@@ -960,7 +961,7 @@ public class DefaultMathTransformFactory extends AbstractFactory implements
Math
              * operation expects two-dimensional source and target CRS. If a given CRS is
three-dimensional, we need
              * a provider variant which will not concatenate a "geographic 3D to 2D" operation
before the Molodensky
              * one. It is worth to perform this check only if the provider is a subclass
of DefaultOperationMethod,
-             * since it needs to override the 'redimension(int, int)' method.
+             * since it needs to override the `redimension(int, int)` method.
              */
             if (method instanceof DefaultOperationMethod && method.getClass() !=
DefaultOperationMethod.class) {
                 final Integer sourceDim = (sourceCS != null) ? sourceCS.getDimension() :
method.getSourceDimensions();
@@ -972,7 +973,7 @@ public class DefaultMathTransformFactory extends AbstractFactory implements
Math
                     }
                 }
             }
-            ensureCompatibleParameters(false);      // Invoke only after we set 'provider'
to its final instance.
+            ensureCompatibleParameters(false);      // Invoke only after we set `provider`
to its final instance.
             /*
              * Get a mask telling us if we need to set parameters for the source and/or target
ellipsoid.
              * This information should preferably be given by the provider. But if the given
provider is
@@ -999,7 +1000,7 @@ public class DefaultMathTransformFactory extends AbstractFactory implements
Math
                     RuntimeException failure = null;
                     if (sourceCS != null) try {
                         ensureCompatibleParameters(true);
-                        final ParameterValue<?> p = parameters.parameter("dim");  
 // Really 'parameters', not 'userParams'.
+                        final ParameterValue<?> p = parameters.parameter("dim");  
 // Really `parameters`, not `userParams`.
                         if (p.getValue() == null) {
                             p.setValue(sourceCS.getDimension());
                         }
@@ -1185,15 +1186,14 @@ public class DefaultMathTransformFactory extends AbstractFactory implements
Math
         ArgumentChecks.ensureNonNull("parameterized", parameterized);
         ArgumentChecks.ensureNonNull("context", context);
         /*
-         * Computes matrix for swapping axis and performing units conversion.
-         * There is one matrix to apply before projection on (longitude,latitude)
-         * coordinates, and one matrix to apply after projection on (easting,northing)
-         * coordinates.
+         * Compute matrices for swapping axis and performing units conversion.
+         * There is one matrix to apply before projection from (λ,φ) coordinates,
+         * and one matrix to apply after projection on (easting,northing) coordinates.
          */
         final Matrix swap1 = context.getMatrix(ContextualParameters.MatrixRole.NORMALIZATION);
         final Matrix swap3 = context.getMatrix(ContextualParameters.MatrixRole.DENORMALIZATION);
         /*
-         * Prepares the concatenation of the matrices computed above and the projection.
+         * Prepare the concatenation of the matrices computed above and the projection.
          * Note that at this stage, the dimensions between each step may not be compatible.
          * For example the projection (step2) is usually two-dimensional while the source
          * coordinate system (step1) may be three-dimensional if it has a height.
@@ -1210,24 +1210,54 @@ public class DefaultMathTransformFactory extends AbstractFactory implements
Math
             step2 = VerticalOffset.postCreate(step2, swap3);
         }
         /*
-         * If the target coordinate system has a height, instructs the projection to pass
-         * the height unchanged from the base CRS to the target CRS. After this block, the
-         * dimensions of 'step2' and 'step3' should match.
+         * If the target coordinate system has a height, instruct the projection to pass
the height unchanged from
+         * the base CRS to the target CRS. After this block, the dimensions of `step2` and
`step3` should match.
+         *
+         * The height is always the last dimension in a normalized EllipdoidalCS. We accept
only a hard-coded list
+         * of dimensions because it is not `MathTransformFactory` job to build a transform
chain in a generic way.
+         * We handle only the cases that are necessary because of the way some operation
methods are provided.
+         * In particular Apache SIS provides only 2D map projections, so 3D projections have
to be "generated"
+         * on the fly. That use case is:
+         *
+         *     - Source CRS: a GeographicCRS (regardless its number of dimension – it will
be addressed in next block)
+         *     - Target CRS: a 3D ProjectedCRS
+         *     - Parameterized transform: a 2D map projection. We need the ellipsoidal height
to passthrough.
+         *
+         * The reverse order (projected source CRS and geographic target CRS) is also accepted
but should be uncommon.
          */
-        final int numTrailingCoordinates = step3.getSourceDimensions() - step2.getTargetDimensions();
+        final int resultDim = step3.getSourceDimensions();
+        final int numTrailingCoordinates = resultDim - step2.getTargetDimensions();
         if (numTrailingCoordinates > 0) {
+            ensureDimensionChangeAllowed(parameterized, context, numTrailingCoordinates,
resultDim);
             step2 = createPassThroughTransform(0, step2, numTrailingCoordinates);
         }
         /*
          * If the source CS has a height but the target CS doesn't, drops the extra coordinates.
-         * After this block, the dimensions of 'step1' and 'step2' should match.
+         * Conversely if the source CS is missing a height, add a height with NaN values.
+         * After this block, the dimensions of `step1` and `step2` should match.
+         *
+         * Note: when adding an ellipsoidal height, we set the height value to NaN instead
than 0 (Earth surface)
+         * because the given `parameterized` transform may be a Molodensky transform or anything
else that could
+         * use the height in its calculation. If we have to add a height, maybe the parameterized
transform is a
+         * 2D Molodensky instead than a 3D Molodensky and the height is just propagated to
the output CS by a
+         * "passthrough" transform. The result is not the same as if a 3D Molodensky was
used in the first place.
+         * A NaN value avoid to give a false sense of accuracy.
          */
         final int sourceDim = step1.getTargetDimensions();
         final int targetDim = step2.getSourceDimensions();
-        if (sourceDim > targetDim) {
-            final Matrix drop = Matrices.createDiagonal(targetDim+1, sourceDim+1);
-            drop.setElement(targetDim, sourceDim, 1); // Element in the lower-right corner.
-            step1 = createConcatenatedTransform(createAffineTransform(drop), step1);
+        final int change    = targetDim - sourceDim;
+        if (change != 0) {
+            if (change > numTrailingCoordinates) {
+                // If we add dimensions, they must be passthrough dimensions.
+                throw new InvalidGeodeticParameterException(canNotAssociateToCS(parameterized,
context));
+            }
+            ensureDimensionChangeAllowed(parameterized, context, change, targetDim);
+            final Matrix resize = Matrices.createZero(targetDim+1, sourceDim+1);
+            for (int j=0; j<targetDim; j++) {
+                resize.setElement(j, Math.min(j, sourceDim), (j < sourceDim) ? 1 : Double.NaN);
+            }
+            resize.setElement(targetDim, sourceDim, 1);     // Element in the lower-right
corner.
+            step1 = createConcatenatedTransform(step1, createAffineTransform(resize));
         }
         MathTransform mt = createConcatenatedTransform(createConcatenatedTransform(step1,
step2), step3);
         /*
@@ -1245,6 +1275,59 @@ public class DefaultMathTransformFactory extends AbstractFactory implements
Math
     }
 
     /**
+     * Checks whether {@link #swapAndScaleAxes(MathTransform, Context)} should accept to
adjust the number of
+     * transform dimensions. Current implementation accepts only addition or removal of ellipsoidal
height,
+     * but future version may expand the list of accepted cases. The intent for this method
is to catch errors
+     * caused by wrong coordinate systems associated to a parameterized transform, keeping
in mind that it is
+     * not {@link DefaultMathTransformFactory} job to handle changes between arbitrary CRS
(those changes are
+     * handled by {@link org.apache.sis.referencing.operation.DefaultCoordinateOperationFactory}
instead).
+     *
+     * <div class="note"><b>Note:</b> the {@code parameterized} transform
is a black box receiving inputs in
+     * any CS and producing outputs in any CS, not necessarily of the same kind. For that
reason, we can not use
+     * {@link CoordinateSystems#swapAndScaleAxes(CoordinateSystem, CoordinateSystem)} between
the normalized CS.
+     * We have to trust that the caller know that the coordinate systems (s)he provided are
correct for the work
+     * done by the transform.</div>
+     *
+     * @param  parameterized  the parameterized transform, for producing an error message
if needed.
+     * @param  context        the source and target coordinate system.
+     * @param  change         number of dimensions to add (if positive) or remove (if negative).
+     * @param  resultDim      number of dimensions after the change.
+     */
+    private static void ensureDimensionChangeAllowed(final MathTransform parameterized,
+            final Context context, final int change, final int resultDim) throws FactoryException
+    {
+        if (Math.abs(change) == 1 && resultDim >= 2 && resultDim <=
3) {
+            if (context.getSourceCS() instanceof EllipsoidalCS ||
+                context.getTargetCS() instanceof EllipsoidalCS)
+            {
+                return;
+            }
+        }
+        throw new InvalidGeodeticParameterException(canNotAssociateToCS(parameterized, context));
+    }
+
+    /**
+     * Creates the error message for a transform that can not be associated with given coordinate
systems.
+     */
+    private static String canNotAssociateToCS(final MathTransform parameterized, final Context
context) {
+        String name = null;
+        if (parameterized instanceof Parameterized) {
+            name = IdentifiedObjects.getDisplayName(((Parameterized) parameterized).getParameterDescriptors(),
null);
+        }
+        if (name == null) {
+            name = Classes.getShortClassName(parameterized);
+        }
+        final StringBuilder b = new StringBuilder();
+        CoordinateSystem cs = context.getSourceCS();
+        if (cs != null) b.append(cs.getDimension()).append("D → ");
+        b.append("tr(").append(parameterized.getSourceDimensions()).append("D → ")
+                     .append(parameterized.getTargetDimensions()).append("D)");
+        cs = context.getTargetCS();
+        if (cs != null) b.append(" → ").append(cs.getDimension()).append('D');
+        return Resources.format(Resources.Keys.CanNotAssociateToCS_2, name, b);
+    }
+
+    /**
      * Creates a transform from a base CRS to a derived CS using the given parameters.
      * If this method needs to set the values of {@code "semi_major"} and {@code "semi_minor"}
parameters,
      * then it sets those values directly on the given {@code parameters} instance – not
on a clone – for
diff --git a/core/sis-referencing/src/test/java/org/apache/sis/referencing/operation/transform/DefaultMathTransformFactoryTest.java
b/core/sis-referencing/src/test/java/org/apache/sis/referencing/operation/transform/DefaultMathTransformFactoryTest.java
index 6d5092b..7203e55 100644
--- a/core/sis-referencing/src/test/java/org/apache/sis/referencing/operation/transform/DefaultMathTransformFactoryTest.java
+++ b/core/sis-referencing/src/test/java/org/apache/sis/referencing/operation/transform/DefaultMathTransformFactoryTest.java
@@ -30,7 +30,6 @@ import org.opengis.referencing.operation.MathTransform;
 import org.opengis.referencing.operation.MathTransformFactory;
 import org.opengis.parameter.ParameterValueGroup;
 import org.apache.sis.parameter.Parameterized;
-import org.apache.sis.referencing.CommonCRS;
 import org.apache.sis.referencing.operation.DefaultConversion;
 import org.apache.sis.referencing.operation.matrix.Matrix2;
 import org.apache.sis.referencing.crs.DefaultProjectedCRS;
@@ -42,7 +41,9 @@ import org.apache.sis.internal.util.Constants;
 import org.apache.sis.measure.Units;
 
 // Test dependencies
+import org.apache.sis.referencing.crs.HardCodedCRS;
 import org.apache.sis.referencing.cs.HardCodedCS;
+import org.apache.sis.referencing.operation.matrix.Matrices;
 import org.apache.sis.test.DependsOnMethod;
 import org.apache.sis.test.DependsOn;
 import org.apache.sis.test.TestCase;
@@ -192,12 +193,12 @@ public final strictfp class DefaultMathTransformFactoryTest extends
TestCase {
          * Gets all map projections and creates a projection using the WGS84 ellipsoid
          * and default parameter values.
          */
+        final MathTransformFactory factory = factory();
         final Map<String,?> dummyName = Collections.singletonMap(DefaultProjectedCRS.NAME_KEY,
"Test");
-        final MathTransformFactory mtFactory = DefaultFactories.forBuildin(MathTransformFactory.class);
-        final Collection<OperationMethod> methods = mtFactory.getAvailableMethods(Projection.class);
+        final Collection<OperationMethod> methods = factory.getAvailableMethods(Projection.class);
         for (final OperationMethod method : methods) {
             final String classification = method.getName().getCode();
-            ParameterValueGroup pg = mtFactory.getDefaultParameters(classification);
+            ParameterValueGroup pg = factory.getDefaultParameters(classification);
             pg.parameter("semi_major").setValue(6377563.396);
             pg.parameter("semi_minor").setValue(6356256.909237285);
             /*
@@ -244,7 +245,7 @@ public final strictfp class DefaultMathTransformFactoryTest extends TestCase
{
             }
             final MathTransform mt;
             try {
-                mt = mtFactory.createParameterizedTransform(pg);
+                mt = factory.createParameterizedTransform(pg);
             } catch (InvalidGeodeticParameterException e) {
                 fail(classification + ": " + e.getLocalizedMessage());
                 continue;
@@ -271,7 +272,7 @@ public final strictfp class DefaultMathTransformFactoryTest extends TestCase
{
              * the one that we specified.
              */
             final DefaultProjectedCRS crs = new DefaultProjectedCRS(dummyName,
-                    CommonCRS.WGS84.normalizedGeographic(),
+                    HardCodedCRS.WGS84,
                     new DefaultConversion(dummyName, method, mt, null),
                     HardCodedCS.PROJECTED);
             final Conversion projection = crs.getConversionFromBase();
@@ -281,17 +282,73 @@ public final strictfp class DefaultMathTransformFactoryTest extends
TestCase {
     }
 
     /**
+     * Tests {@link DefaultMathTransformFactory#swapAndScaleAxes(MathTransform, DefaultMathTransformFactory.Context)}
+     * with different number of dimensions.
+     *
+     * @throws FactoryException if the transform construction failed.
+     */
+    @Test
+    public void testSwapAndScaleAxes() throws FactoryException {
+        final DefaultMathTransformFactory factory = factory();
+        final DefaultMathTransformFactory.Context context = new DefaultMathTransformFactory.Context();
+        context.setSource(HardCodedCS.GEODETIC_3D);
+        context.setTarget(HardCodedCS.CARTESIAN_3D);
+        /*
+         * Simulate a case where the parameterized transform is a two-dimensional map projection,
+         * but the input and output CRS are three-dimensional geographic and projected CRS
respectively.
+         */
+        MathTransform mt = factory.swapAndScaleAxes(MathTransforms.identity(2), context);
+        assertEquals(3, mt.getSourceDimensions());
+        assertEquals(3, mt.getTargetDimensions());
+        assertTrue(mt.isIdentity());
+        /*
+         * Transform from 3D to 2D. Height dimension is dropped.
+         */
+        context.setSource(HardCodedCS.GEODETIC_3D);
+        context.setTarget(HardCodedCS.GEODETIC_2D);
+        mt = factory.swapAndScaleAxes(MathTransforms.identity(2), context);
+        assertMatrixEquals("3D → 2D", Matrices.create(3, 4, new double[] {
+            1, 0, 0, 0,
+            0, 1, 0, 0,
+            0, 0, 0, 1
+        }), MathTransforms.getMatrix(mt), STRICT);
+        /*
+         * Transform from 2D to 3D. Coordinate values in the height dimension are unknown
(NaN).
+         */
+        context.setSource(HardCodedCS.GEODETIC_2D);
+        context.setTarget(HardCodedCS.GEODETIC_3D);
+        mt = factory.swapAndScaleAxes(MathTransforms.identity(2), context);
+        assertMatrixEquals("2D → 3D", Matrices.create(4, 3, new double[] {
+            1, 0, 0,
+            0, 1, 0,
+            0, 0, Double.NaN,
+            0, 0, 1
+        }), MathTransforms.getMatrix(mt), STRICT);
+        /*
+         * Test error message when adding a dimension that is not ellipsoidal height.
+         */
+        context.setSource(HardCodedCS.CARTESIAN_2D);
+        context.setTarget(HardCodedCS.CARTESIAN_3D);
+        try {
+            factory.swapAndScaleAxes(MathTransforms.identity(2), context);
+            fail("Should not have accepted the given coordinate systems.");
+        } catch (InvalidGeodeticParameterException e) {
+            final String message = e.getMessage();
+            assertTrue(message, message.contains("2D → tr(2D → 2D) → 3D"));
+        }
+    }
+
+    /**
      * Tests {@link DefaultMathTransformFactory#caching(boolean)}.
      */
     @Test
     public void testCaching() {
-        final DefaultMathTransformFactory mtFactory = DefaultFactories.forBuildin(
-                    MathTransformFactory.class, DefaultMathTransformFactory.class);
-        final DefaultMathTransformFactory caching = mtFactory.caching(false);
+        final DefaultMathTransformFactory factory = factory();
+        final DefaultMathTransformFactory caching = factory.caching(false);
 
-        assertNotSame(mtFactory, caching);
-        assertSame   (mtFactory, mtFactory.caching(true));
-        assertSame   (mtFactory,   caching.caching(true));
+        assertNotSame(factory, caching);
+        assertSame   (factory, factory.caching(true));
+        assertSame   (factory,   caching.caching(true));
         assertSame   (caching,     caching.caching(false));
     }
 }


Mime
View raw message