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 022a1f3fffaf04cc3b2bdf75829287a678889a3a
Author: Martin Desruisseaux <martin.desruisse...@geomatys.com>
AuthorDate: Wed Aug 11 15:46:36 2021 +0200
Consolidation:
- Safer rounding of subsampling values.
- Add test and clarifications in Javadoc.
---
.../apache/sis/coverage/grid/GridDerivation.java | 40 ++++++++++++++++------
.../sis/coverage/grid/GridDerivationTest.java | 24 +++++++++++++
.../org/apache/sis/storage/geotiff/GeoTIFF.java | 2 +-
.../apache/sis/storage/geotiff/GeoTiffStore.java | 2 +-
.../sis/internal/storage/AbstractResource.java | 7 ++++
.../apache/sis/storage/event/StoreListeners.java | 6 ++--
6 files changed, 66 insertions(+), 15 deletions(-)
diff --git
a/core/sis-feature/src/main/java/org/apache/sis/coverage/grid/GridDerivation.java
b/core/sis-feature/src/main/java/org/apache/sis/coverage/grid/GridDerivation.java
index 445e2ff..0c9c26a 100644
---
a/core/sis-feature/src/main/java/org/apache/sis/coverage/grid/GridDerivation.java
+++
b/core/sis-feature/src/main/java/org/apache/sis/coverage/grid/GridDerivation.java
@@ -1277,7 +1277,7 @@ public class GridDerivation {
/**
* Rounds a subsampling value according the current {@link RoundingMode}.
* If a {@link #chunkSize} has been specified, then the subsampling will
be a divisor of that size.
- * This is necessary for avoiding a drift in subsampled pixel coordinates
computed from tile coordinates.
+ * This is necessary for avoiding a drift of subsampled pixel coordinates
computed from tile coordinates.
*
* <div class="note"><b>Drift example:</b>
* if the tile size is 16 pixels and the subsampling is 3, then the
subsampled tile size is ⌊16/3⌋ = 5 pixels.
@@ -1306,8 +1306,8 @@ public class GridDerivation {
switch (rounding) {
default: throw new AssertionError(rounding);
case NEAREST: subsampling = (int) Math.min(Math.round(scale),
Integer.MAX_VALUE); break;
- case CONTAINED: // Assume user wants more data in source
(ENCLOSING) or target (CONTAINED) grid.
- case ENCLOSING: subsampling = (int) Math.nextUp(scale); break;
+ case CONTAINED: subsampling = (int) Math.ceil(scale -
tolerance(dimension)); break;
+ case ENCLOSING: subsampling = (int) (scale +
tolerance(dimension)); break;
}
if (subsampling <= 1) {
return 1;
@@ -1321,24 +1321,42 @@ public class GridDerivation {
/*
* `binarySearch(…)` should never find an exact match,
otherwise (size % r) would have been zero.
* Furthermore `i` should never be 0 because divisors[0] = 1,
which can not be selected if r > 1.
- * We nevertheless check for (i > 0) as a paranoiac safety.
+ * We do not check `if (i > 0)` "as a safety" because client
code such as `TiledGridCoverage`
+ * will behave erratically if this method does not fulfill its
contract (i.e. find a divisor).
+ * It is better to know now if there is any problem here.
*/
- if (i > 0) {
- int s = divisors[i-1];
- if (rounding == GridRoundingMode.NEAREST && i <
divisors.length) {
- final int above = divisors[i];
- if (above - r < r - s) {
- s = above;
+ int s = divisors[i-1];
+ if (i < divisors.length) {
+ switch (rounding) {
+ case CONTAINED: {
+ s = divisors[i];
+ break;
+ }
+ case NEAREST: {
+ final int above = divisors[i];
+ if (above - r < r - s) {
+ s = above;
+ }
+ break;
}
}
- return s + (subsampling - r);
}
+ return s + (subsampling - r);
}
}
return subsampling;
}
/**
+ * Returns a tolerance factor for comparing scale factors in the given
dimension.
+ * The tolerance is such that the errors of pixel coordinates computed
using the
+ * scale factor should not be greater than 0.5 pixel.
+ */
+ private double tolerance(final int dimension) {
+ return (base.extent != null) ? 0.5 / base.extent.getSize(dimension,
false) : 0;
+ }
+
+ /**
* Returns the offsets to be subtracted from pixel coordinates before
subsampling.
* In a conversion from <em>derived</em> grid to {@linkplain #base} grid
coordinates
* (the opposite direction of subsampling), the offset is the value to add
after
diff --git
a/core/sis-feature/src/test/java/org/apache/sis/coverage/grid/GridDerivationTest.java
b/core/sis-feature/src/test/java/org/apache/sis/coverage/grid/GridDerivationTest.java
index 3573c30..28887b0 100644
---
a/core/sis-feature/src/test/java/org/apache/sis/coverage/grid/GridDerivationTest.java
+++
b/core/sis-feature/src/test/java/org/apache/sis/coverage/grid/GridDerivationTest.java
@@ -374,6 +374,30 @@ public final strictfp class GridDerivationTest extends
TestCase {
}
/**
+ * Tests {@link GridDerivation#subgrid(GridExtent)} with a null "grid to
CRS" transform.
+ */
+ @Test
+ public void testSubgridWithoutTransform() {
+ GridExtent base = new GridExtent(null, new long[] {100, 200}, new
long[] {300, 350}, true);
+ GridExtent query = new GridExtent(null, new long[] {120, 180}, new
long[] {280, 360}, true);
+ GridGeometry result = new GridGeometry(base, null, null,
null).derive().subgrid(query).build();
+ assertExtentEquals(new long[] {120, 200}, new long[] {280, 350},
result.getExtent());
+ assertFalse(result.isDefined(GridGeometry.GRID_TO_CRS));
+ assertFalse(result.isDefined(GridGeometry.CRS));
+ assertFalse(result.isDefined(GridGeometry.RESOLUTION));
+ /*
+ * Try again with a subsampling. We can get the subsampling
information back as the resolution.
+ * Note that there is no way with current API to get the subsampling
offsets.
+ */
+ result = new GridGeometry(base, null, null,
null).derive().subgrid(query, 3, 5).build();
+ assertExtentEquals(new long[] {40, 40}, new long[] {93, 70},
result.getExtent());
+ assertFalse(result.isDefined(GridGeometry.GRID_TO_CRS));
+ assertFalse(result.isDefined(GridGeometry.CRS));
+ assertTrue (result.isDefined(GridGeometry.RESOLUTION));
+ assertArrayEquals(new double[] {3, 5}, result.getResolution(false),
STRICT);
+ }
+
+ /**
* Tests {@link GridDerivation#slice(DirectPosition)}.
*/
@Test
diff --git
a/storage/sis-geotiff/src/main/java/org/apache/sis/storage/geotiff/GeoTIFF.java
b/storage/sis-geotiff/src/main/java/org/apache/sis/storage/geotiff/GeoTIFF.java
index 3f9d7da..baa5a9e 100644
---
a/storage/sis-geotiff/src/main/java/org/apache/sis/storage/geotiff/GeoTIFF.java
+++
b/storage/sis-geotiff/src/main/java/org/apache/sis/storage/geotiff/GeoTIFF.java
@@ -49,7 +49,7 @@ abstract class GeoTIFF implements Closeable {
* The locale to use for parsers or formatter. This is
<strong>not</strong> the locale
* for warnings or other messages emitted to the users.
*/
- static final Locale LOCALE = Locale.US;
+ private static final Locale LOCALE = Locale.US;
/**
* The magic number for big-endian TIFF files or little-endian TIFF files.
diff --git
a/storage/sis-geotiff/src/main/java/org/apache/sis/storage/geotiff/GeoTiffStore.java
b/storage/sis-geotiff/src/main/java/org/apache/sis/storage/geotiff/GeoTiffStore.java
index 4a234d1..98dd0ed 100644
---
a/storage/sis-geotiff/src/main/java/org/apache/sis/storage/geotiff/GeoTiffStore.java
+++
b/storage/sis-geotiff/src/main/java/org/apache/sis/storage/geotiff/GeoTiffStore.java
@@ -228,7 +228,7 @@ public class GeoTiffStore extends DataStore implements
Aggregate {
/*
* Add the filename as an identifier only if the input was
something convertible to URI (URL, File or Path),
* otherwise reader.input.filename may not be useful; it may be
just the InputStream classname. If the TIFF
- * file did not specified any ImageDescription tag, then we will
had the filename as a title instead than an
+ * file did not specified any ImageDescription tag, then we will
add the filename as a title instead than an
* identifier because the title is mandatory in ISO 19115 metadata.
*/
getIdentifier().ifPresent((id) ->
builder.addTitleOrIdentifier(id.toString(), MetadataBuilder.Scope.ALL));
diff --git
a/storage/sis-storage/src/main/java/org/apache/sis/internal/storage/AbstractResource.java
b/storage/sis-storage/src/main/java/org/apache/sis/internal/storage/AbstractResource.java
index a1c8eba..c2def88 100644
---
a/storage/sis-storage/src/main/java/org/apache/sis/internal/storage/AbstractResource.java
+++
b/storage/sis-storage/src/main/java/org/apache/sis/internal/storage/AbstractResource.java
@@ -76,6 +76,12 @@ public class AbstractResource extends StoreListeners
implements Resource {
* Returns the resource persistent identifier if available.
* The default implementation returns an empty value.
* Subclasses are strongly encouraged to override if they can provide a
value.
+ *
+ * <p>Note that the default implementation of {@link
#createMetadata(MetadataBuilder)} uses this identifier
+ * for initializing the {@code metadata/identificationInfo/citation/title}
property. So it is generally not
+ * useful to fallback on metadata if the identifier is empty.</p>
+ *
+ * @see org.apache.sis.internal.storage.StoreUtilities#getLabel(Resource)
*/
@Override
public Optional<GenericName> getIdentifier() throws DataStoreException {
@@ -120,6 +126,7 @@ public class AbstractResource extends StoreListeners
implements Resource {
* @throws DataStoreException if an error occurred while reading metadata
from the data store.
*/
protected void createMetadata(final MetadataBuilder metadata) throws
DataStoreException {
+ // Note: title is mandatory in ISO metadata, contrarily to the
identifier.
getIdentifier().ifPresent((name) -> metadata.addTitle(new
Sentence(name)));
getEnvelope().ifPresent((envelope) -> {
try {
diff --git
a/storage/sis-storage/src/main/java/org/apache/sis/storage/event/StoreListeners.java
b/storage/sis-storage/src/main/java/org/apache/sis/storage/event/StoreListeners.java
index 8c0819a..55d3d69 100644
---
a/storage/sis-storage/src/main/java/org/apache/sis/storage/event/StoreListeners.java
+++
b/storage/sis-storage/src/main/java/org/apache/sis/storage/event/StoreListeners.java
@@ -31,6 +31,7 @@ import org.apache.sis.util.logging.Logging;
import org.apache.sis.util.resources.Vocabulary;
import org.apache.sis.internal.storage.StoreResource;
import org.apache.sis.internal.storage.StoreUtilities;
+import org.apache.sis.internal.storage.AbstractResource;
import org.apache.sis.storage.DataStoreProvider;
import org.apache.sis.storage.DataStore;
import org.apache.sis.storage.Resource;
@@ -84,6 +85,7 @@ public class StoreListeners implements Localized {
/**
* The declared source of events. This is not necessarily the real source,
* but this is the source that the implementer wants to declare as public
API.
+ * Never {@code null} but may be {@code this}.
*/
private final Resource source;
@@ -214,7 +216,7 @@ public class StoreListeners implements Localized {
* This is used as a convenience by AbstractResource internal class.
We need this hack
* because subclasses can not reference `this` before super-class
constructor completed.
*/
- if (source == null && this instanceof Resource) {
+ if (source == null && this instanceof AbstractResource) {
source = (Resource) this;
} else {
ArgumentChecks.ensureNonNull("source", source);
@@ -226,7 +228,7 @@ public class StoreListeners implements Localized {
/**
* Returns the source of events. This value is specified at construction
time.
*
- * @return the source of events.
+ * @return the source of events. Never {@code null} but may be {@code
this}.
*/
public Resource getSource() {
return source;
