You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@sis.apache.org by de...@apache.org on 2021/07/07 11:06:14 UTC
[sis] branch geoapi-4.0 updated (0df0bc4 -> 99cafb6)
This is an automated email from the ASF dual-hosted git repository.
desruisseaux pushed a change to branch geoapi-4.0
in repository https://gitbox.apache.org/repos/asf/sis.git.
from 0df0bc4 Fix a wrong transform concatenation occurring when matrix coefficients are very close to zero. Bug identified by Johann Sorel.
new 04965b4 ProjectiveTransform wrongly optimized to translation when the scale coefficient is zero. Bug identified by Johann Sorel.
new 99cafb6 Avoid the use of `FeatureTypeBuilder` for keeping tests more isolated (i.e. for not being impacted by potential bugs in FeatureTypeBuilder). Use `TestUtilities.getSingleton(…)` method for checking the result. Keep `verifyCopyrightParsing(…)` close to the place where it is used.
The 2 revisions listed above as "new" are entirely new to this
repository and will be described in separate emails. The revisions
listed as "add" were already present in the repository and have only
been added to this reference.
Summary of changes:
.../operation/transform/ProjectiveTransform.java | 14 ++-
.../transform/ProjectiveTransformTest.java | 28 +++++-
.../sis/internal/storage/MetadataBuilderTest.java | 101 ++++++++++++---------
3 files changed, 90 insertions(+), 53 deletions(-)
[sis] 01/02: ProjectiveTransform wrongly optimized to translation
when the scale coefficient is zero. Bug identified by Johann Sorel.
Posted by de...@apache.org.
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 04965b44c2d6b1ccc153ffb1b4f479c1848bef1c
Author: Martin Desruisseaux <ma...@geomatys.com>
AuthorDate: Wed Jul 7 13:04:21 2021 +0200
ProjectiveTransform wrongly optimized to translation when the scale coefficient is zero.
Bug identified by Johann Sorel.
---
.../operation/transform/ProjectiveTransform.java | 14 +++++------
.../transform/ProjectiveTransformTest.java | 28 ++++++++++++++++++++--
2 files changed, 32 insertions(+), 10 deletions(-)
diff --git a/core/sis-referencing/src/main/java/org/apache/sis/referencing/operation/transform/ProjectiveTransform.java b/core/sis-referencing/src/main/java/org/apache/sis/referencing/operation/transform/ProjectiveTransform.java
index f8d0175..da34939 100644
--- a/core/sis-referencing/src/main/java/org/apache/sis/referencing/operation/transform/ProjectiveTransform.java
+++ b/core/sis-referencing/src/main/java/org/apache/sis/referencing/operation/transform/ProjectiveTransform.java
@@ -116,14 +116,12 @@ class ProjectiveTransform extends AbstractLinearTransform implements ExtendedPre
final int lastColumn = numCol - 1;
for (int i=0; i<n; i++) {
final double v = elt[i];
- if (v != 0) {
- final int row = (i / numCol);
- final int col = (i % numCol);
- isScale &= (col == row);
- isTranslation &= (col == lastColumn) || (col == row && v == 1);
- if (!(isScale | isTranslation)) {
- return this;
- }
+ final int row = (i / numCol);
+ final int col = (i % numCol);
+ isScale &= (col == row) || (v == 0);
+ isTranslation &= (col == lastColumn) || (v == (col == row ? 1 : 0));
+ if (!(isScale | isTranslation)) {
+ return this;
}
}
if (isTranslation) {
diff --git a/core/sis-referencing/src/test/java/org/apache/sis/referencing/operation/transform/ProjectiveTransformTest.java b/core/sis-referencing/src/test/java/org/apache/sis/referencing/operation/transform/ProjectiveTransformTest.java
index bd86e1c..45d13a5 100644
--- a/core/sis-referencing/src/test/java/org/apache/sis/referencing/operation/transform/ProjectiveTransformTest.java
+++ b/core/sis-referencing/src/test/java/org/apache/sis/referencing/operation/transform/ProjectiveTransformTest.java
@@ -24,6 +24,7 @@ import org.opengis.referencing.operation.MathTransformFactory;
import org.opengis.referencing.operation.TransformException;
import org.apache.sis.referencing.operation.matrix.Matrices;
import org.apache.sis.referencing.operation.matrix.MatrixSIS;
+import org.apache.sis.referencing.operation.matrix.Matrix2;
import org.apache.sis.internal.referencing.provider.Affine;
import org.apache.sis.parameter.Parameterized;
@@ -34,6 +35,7 @@ import org.apache.sis.test.DependsOn;
import org.junit.runner.RunWith;
import org.junit.After;
import org.junit.Test;
+import org.opengis.test.Assert;
import static org.opengis.test.Assert.*;
// Branch-dependent imports
@@ -47,7 +49,7 @@ import org.opengis.test.referencing.AffineTransformTest;
* this time with NaN values.
*
* @author Martin Desruisseaux (Geomatys)
- * @version 0.7
+ * @version 1.1
* @since 0.5
* @module
*/
@@ -55,6 +57,11 @@ import org.opengis.test.referencing.AffineTransformTest;
@DependsOn({AbstractMathTransformTest.class, ScaleTransformTest.class})
public strictfp class ProjectiveTransformTest extends AffineTransformTest {
/**
+ * Tolerance factor for strict comparisons.
+ */
+ private static final double STRICT = 0;
+
+ /**
* For {@link LinearTransformTest} constructor only.
*/
ProjectiveTransformTest(final MathTransformFactory factory) {
@@ -80,7 +87,7 @@ public strictfp class ProjectiveTransformTest extends AffineTransformTest {
* Opportunistically tests ScaledTransform together with ProjectiveTransform.
* We takes ScaledTransform as a reference implementation since it is simpler.
*/
- tr = new TransformResultComparator(tr, pt, 0);
+ tr = new TransformResultComparator(tr, pt, STRICT);
}
return tr;
}
@@ -132,6 +139,23 @@ public strictfp class ProjectiveTransformTest extends AffineTransformTest {
}
/**
+ * Tests {@link ProjectiveTransform#optimize()} when the matrix defines a constant value.
+ * Older SIS versions wrongly optimized this case as a translation.
+ *
+ * @throws TransformException if a coordinate conversion failed.
+ *
+ * @since 1.1
+ */
+ @Test
+ public void testOptimizeConstant() throws TransformException {
+ matrix = new Matrix2(0, 10, 0, 1);
+ transform = new ProjectiveTransform(matrix).optimize();
+ Assert.assertMatrixEquals("Transform shall use the given matrix unmodified.",
+ matrix, ((LinearTransform) transform).getMatrix(), STRICT);
+ verifyConsistency(1, 2, 3, -3, -2, -1);
+ }
+
+ /**
* {@code true} if {@link #ensureImplementRightInterface()} should skip its check for a transform
* of the given dimension. {@code ProjectiveTransformTest} needs to skip the case for dimension 1
* because there is no {@code ProjectiveTransform1D} class. However {@link LinearTransformTest}
[sis] 02/02: Avoid the use of `FeatureTypeBuilder` for keeping tests more isolated (i.e. for not being impacted by potential bugs in FeatureTypeBuilder). Use `TestUtilities.getSingleton(…)` method for checking the result. Keep `verifyCopyrightParsing(…)` close to the place where it is used.
Posted by de...@apache.org.
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 99cafb6339df2837cc7bf0c5a505e8ad2f141321
Author: Martin Desruisseaux <ma...@geomatys.com>
AuthorDate: Wed Jul 7 12:03:54 2021 +0200
Avoid the use of `FeatureTypeBuilder` for keeping tests more isolated
(i.e. for not being impacted by potential bugs in FeatureTypeBuilder).
Use `TestUtilities.getSingleton(…)` method for checking the result.
Keep `verifyCopyrightParsing(…)` close to the place where it is used.
---
.../sis/internal/storage/MetadataBuilderTest.java | 101 ++++++++++++---------
1 file changed, 58 insertions(+), 43 deletions(-)
diff --git a/storage/sis-storage/src/test/java/org/apache/sis/internal/storage/MetadataBuilderTest.java b/storage/sis-storage/src/test/java/org/apache/sis/internal/storage/MetadataBuilderTest.java
index bc8a8e3..616f431 100644
--- a/storage/sis-storage/src/test/java/org/apache/sis/internal/storage/MetadataBuilderTest.java
+++ b/storage/sis-storage/src/test/java/org/apache/sis/internal/storage/MetadataBuilderTest.java
@@ -16,28 +16,33 @@
*/
package org.apache.sis.internal.storage;
-import org.apache.sis.feature.builder.FeatureTypeBuilder;
-import org.apache.sis.metadata.iso.DefaultMetadata;
-import org.opengis.feature.FeatureType;
+import java.util.Collections;
+import org.opengis.util.GenericName;
+import org.opengis.metadata.citation.Citation;
+import org.opengis.metadata.content.ContentInformation;
+import org.opengis.metadata.content.FeatureCatalogueDescription;
+import org.opengis.metadata.content.FeatureTypeInfo;
import org.opengis.metadata.constraint.LegalConstraints;
import org.opengis.metadata.constraint.Restriction;
-import org.opengis.metadata.citation.Citation;
+import org.apache.sis.metadata.iso.DefaultMetadata;
+import org.apache.sis.feature.DefaultFeatureType;
import org.apache.sis.test.TestCase;
import org.junit.Test;
-import org.opengis.metadata.content.FeatureCatalogueDescription;
-import org.opengis.metadata.content.FeatureTypeInfo;
-import org.opengis.util.GenericName;
import static org.apache.sis.test.MetadataAssert.*;
import static org.apache.sis.test.TestUtilities.date;
import static org.apache.sis.test.TestUtilities.getSingleton;
+// Branch-dependent imports
+import org.opengis.feature.FeatureType;
+
/**
* Tests {@link MetadataBuilder}.
*
* @author Martin Desruisseaux (Geomatys)
- * @version 0.8
+ * @author Alexis Manin (Geomatys)
+ * @version 1.1
* @since 0.8
* @module
*/
@@ -69,70 +74,80 @@ public final strictfp class MetadataBuilderTest extends TestCase {
verifyCopyrightParsing("(C) COPYRIGHT 1992 John Smith.");
}
+ /**
+ * Verifies the metadata that contains the result of parsing a copyright statement.
+ * Should contains the "John Smith" name and 1992 year.
+ *
+ * @param notice the copyright statement to parse.
+ */
+ private static void verifyCopyrightParsing(final String notice) {
+ final MetadataBuilder builder = new MetadataBuilder();
+ builder.parseLegalNotice(notice);
+ final LegalConstraints constraints = (LegalConstraints) getSingleton(getSingleton(
+ builder.build(false).getIdentificationInfo()).getResourceConstraints());
+
+ assertEquals("useConstraints", Restriction.COPYRIGHT, getSingleton(constraints.getUseConstraints()));
+ final Citation ref = getSingleton(constraints.getReferences());
+ assertTitleEquals("reference.title", notice, ref);
+ assertPartyNameEquals("reference.citedResponsibleParty", "John Smith", ref);
+ assertEquals("date", date("1992-01-01 00:00:00"), getSingleton(ref.getDates()).getDate());
+ }
+
+ /**
+ * Tests {@link MetadataBuilder#addFeatureType(FeatureType, long)}.
+ *
+ * @todo Combine the 4 tests in a single one for leveraging the same {@link DefaultFeatureType} instance?
+ * It would be consistent with {@link #testParseLegalNotice()}, and the error message in those tests
+ * are already quite clear.
+ */
@Test
public void negative_feature_count_are_ignored() {
verifyFeatureInstanceCount("Feature count should not be written if it is negative", null, -1);
}
+ /**
+ * Tests {@link MetadataBuilder#addFeatureType(FeatureType, long)}.
+ */
@Test
public void no_overflow_on_feature_count() {
verifyFeatureInstanceCount("Feature count should be limited to maximum 32bit integer value", Integer.MAX_VALUE, 7_000_000_000L);
}
+ /**
+ * Tests {@link MetadataBuilder#addFeatureType(FeatureType, long)}.
+ */
@Test
public void verify_feature_count_is_written() {
verifyFeatureInstanceCount("Feature count should be written as is", 42, 42);
}
+ /**
+ * Tests {@link MetadataBuilder#addFeatureType(FeatureType, long)}.
+ */
@Test
public void feature_count_should_be_ignored_when_it_is_zero() {
verifyFeatureInstanceCount("Feature count should not be written if it is 0", null, 0);
}
/**
- * Create a new simple metadata with a single simple feature type and the given
- * {@link FeatureTypeInfo#getFeatureInstanceCount() feature instance count}. Then, assert that the value in the
- * built metadata is compliant with a given control value.
+ * Creates a new simple metadata with a single simple feature type and the given
+ * {@linkplain FeatureTypeInfo#getFeatureInstanceCount() feature instance count}.
+ * Then, asserts that the value in the built metadata is compliant with a given control value.
*
- * @param expected The feature instance count value we want to see in the metadata (control value)
- * @param valueToInsert The value to send to the metadata builder.
- *
- * @see MetadataBuilder#addFeatureType(FeatureType, long)
+ * @param expected the feature instance count value we want to see in the metadata (control value).
+ * @param valueToInsert the value to send to the metadata builder.
*/
private static void verifyFeatureInstanceCount(final String errorMessage, final Integer expected, final long valueToInsert) {
- final FeatureType dataType = new FeatureTypeBuilder()
- .setName("Test type")
- .build();
+ final DefaultFeatureType dataType = new DefaultFeatureType(
+ Collections.singletonMap(DefaultFeatureType.NAME_KEY, "Test type"), false, null);
final MetadataBuilder builder = new MetadataBuilder();
final GenericName name = builder.addFeatureType(dataType, valueToInsert);
assertNotNull(name);
final DefaultMetadata metadata = builder.build(true);
- final FeatureTypeInfo info = metadata.getContentInfo().stream()
- .filter(it -> it instanceof FeatureCatalogueDescription)
- .flatMap(it -> ((FeatureCatalogueDescription) it).getFeatureTypeInfo().stream())
- .reduce((v1, v2) -> { throw new AssertionError("A single feature type info is expected"); })
- .orElseThrow(() -> new AssertionError("A single feature type info is expected"));
-
+ final ContentInformation content = getSingleton(metadata.getContentInfo());
+ assertInstanceOf("Metadata.contentInfo", FeatureCatalogueDescription.class, content);
+ final FeatureTypeInfo info = getSingleton(((FeatureCatalogueDescription) content).getFeatureTypeInfo());
assertEquals(errorMessage, expected, info.getFeatureInstanceCount());
}
-
- /**
- * Verifies the metadata that contains the result of parsing a copyright statement.
- * Should contains the "John Smith" name and 1992 year.
- *
- * @param notice the copyright statement to parse.
- */
- private static void verifyCopyrightParsing(final String notice) {
- final MetadataBuilder builder = new MetadataBuilder();
- builder.parseLegalNotice(notice);
- final LegalConstraints constraints = (LegalConstraints) getSingleton(getSingleton(
- builder.build(false).getIdentificationInfo()).getResourceConstraints());
-
- assertEquals("useConstraints", Restriction.COPYRIGHT, getSingleton(constraints.getUseConstraints()));
- final Citation ref = getSingleton(constraints.getReferences());
- assertTitleEquals("reference.title", notice, ref);
- assertPartyNameEquals("reference.citedResponsibleParty", "John Smith", ref);
- assertEquals("date", date("1992-01-01 00:00:00"), getSingleton(ref.getDates()).getDate());
- }
}