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 58e7210b1bce7c13d91b15850c2d1589226796f1 Author: Martin Desruisseaux <[email protected]> AuthorDate: Thu Feb 5 11:47:27 2026 +0100 Add documentation and test in `DefaultEvaluator` in attempt to identify the cause of a random JUnit test failure during parallel execution. --- .../apache/sis/coverage/grid/DefaultEvaluator.java | 148 ++++++++++++--------- .../sis/coverage/grid/ValuesAtPointIterator.java | 13 +- .../sis/coverage/grid/DefaultEvaluatorTest.java | 48 +++++++ 3 files changed, 141 insertions(+), 68 deletions(-) diff --git a/endorsed/src/org.apache.sis.feature/main/org/apache/sis/coverage/grid/DefaultEvaluator.java b/endorsed/src/org.apache.sis.feature/main/org/apache/sis/coverage/grid/DefaultEvaluator.java index 3662d73a6e..d5f815a7df 100644 --- a/endorsed/src/org.apache.sis.feature/main/org/apache/sis/coverage/grid/DefaultEvaluator.java +++ b/endorsed/src/org.apache.sis.feature/main/org/apache/sis/coverage/grid/DefaultEvaluator.java @@ -22,6 +22,7 @@ import java.util.Arrays; import java.util.TreeMap; import java.util.Objects; import java.util.Collection; +import java.util.Spliterator; import java.util.function.IntFunction; import java.util.stream.Stream; import java.util.stream.StreamSupport; @@ -363,6 +364,27 @@ abstract class DefaultEvaluator implements GridCoverage.Evaluator { */ @Override public Stream<double[]> stream(final Collection<? extends DirectPosition> points, final boolean parallel) { + try { + return StreamSupport.stream(spliterator(points), parallel); + } catch (CannotEvaluateException ex) { + throw ex; + } catch (RuntimeException | FactoryException | TransformException ex) { + throw new CannotEvaluateException(ex.getMessage(), ex); + } + } + + /** + * Returns an iterator over the sample values for each point of the given collection. + * The iteration happens in the same order as in the given iterator. + * + * @param points the positions where to evaluate. + * @return iterator over the sample values at the specified positions. + * @throws FactoryException if an exception occurred while search an operation to the <abbr>CRS</abbr> of a point. + * @throws TransformException if a coordinate transformation failed. + */ + final Spliterator<double[]> spliterator(final Collection<? extends DirectPosition> points) + throws FactoryException, TransformException + { final GridCoverage coverage = getCoverage(); final int dimension = coverage.gridGeometry.getDimension(); double[] coordinates = ArraysExt.EMPTY_DOUBLE; @@ -372,77 +394,71 @@ abstract class DefaultEvaluator implements GridCoverage.Evaluator { int inputCoordinateOffset = 0; int firstCoordToTransform = 0; int numPointsToTransform = 0; - try { - /* - * Convert the "real world " coordinates to grid coordinates. - * The CRS of each point is verified. Consecutive points with - * the same CRS will be transformed in a single operation. - */ - for (final DirectPosition point : points) { - if (crs != (crs = point.getCoordinateReferenceSystem())) { - if (numPointsToTransform > 0) { // Because `toGrid` may be null. - assert toGrid.getTargetDimensions() == dimension; - toGrid.transform(coordinates, firstCoordToTransform, - coordinates, firstCoordToTransform, - numPointsToTransform); - } - firstCoordToTransform += numPointsToTransform * dimension; - inputCoordinateOffset = firstCoordToTransform; - numPointsToTransform = 0; - toGrid = getInputToGrid(crs); - srcDim = toGrid.getSourceDimensions(); - } - int offset = inputCoordinateOffset; - if ((inputCoordinateOffset += srcDim) > coordinates.length) { - int n = firstCoordToTransform / dimension; // Number of points already transformed. - n = points.size() - n + numPointsToTransform; // Number of points left to transform. - coordinates = Arrays.copyOf(coordinates, Math.multiplyExact(n, Math.max(srcDim, dimension)) + offset); - } - for (int i=0; i<srcDim; i++) { - coordinates[offset++] = point.getCoordinate(i); + /* + * Convert the "real world " coordinates to grid coordinates. + * The CRS of each point is verified. Consecutive points with + * the same CRS will be transformed in a single operation. + */ + for (final DirectPosition point : points) { + if (crs != (crs = point.getCoordinateReferenceSystem())) { + if (numPointsToTransform > 0) { // Because `toGrid` may be null. + assert toGrid.getTargetDimensions() == dimension; + toGrid.transform(coordinates, firstCoordToTransform, + coordinates, firstCoordToTransform, + numPointsToTransform); } - numPointsToTransform++; + firstCoordToTransform += numPointsToTransform * dimension; + inputCoordinateOffset = firstCoordToTransform; + numPointsToTransform = 0; + toGrid = getInputToGrid(crs); + srcDim = toGrid.getSourceDimensions(); } - /* - * Transforms the remaining sequence of points. - * Actually, in most cases, all points are transformed here. - */ - if (numPointsToTransform > 0) { // Because `toGrid` may be null. - assert toGrid.getTargetDimensions() == dimension; - toGrid.transform(coordinates, firstCoordToTransform, - coordinates, firstCoordToTransform, - numPointsToTransform); + int offset = inputCoordinateOffset; + if ((inputCoordinateOffset += srcDim) > coordinates.length) { + int n = firstCoordToTransform / dimension; // Number of points already transformed. + n = points.size() - n + numPointsToTransform; // Number of points left to transform. + coordinates = Arrays.copyOf(coordinates, Math.multiplyExact(n, Math.max(srcDim, dimension)) + offset); } - final int numPoints = firstCoordToTransform / dimension + numPointsToTransform; - postTransform(coordinates, 0, numPoints); - /* - * Create the iterator. The `ValuesAtPointIterator.create(…)` method will identify the slices in - * n-dimensional coverage, get the rendered images for the regions of interest and get the tiles. - */ - final IntFunction<PointOutsideCoverageException> ifOutside; - if (nullIfOutside) { - ifOutside = null; - } else { - final var listOfPoints = (points instanceof List<?>) ? (List<? extends DirectPosition>) points : null; - ifOutside = (index) -> { - DirectPosition point = null; - if (listOfPoints != null) try { - point = listOfPoints.get(index); - } catch (IndexOutOfBoundsException e) { - recoverableException("pointOutsideCoverage", e); - } - if (point != null) { - return pointOutsideCoverage(point); - } - return new PointOutsideCoverageException(Resources.format(Resources.Keys.PointOutsideCoverageDomain_1, "#" + index)); - }; + for (int i=0; i<srcDim; i++) { + coordinates[offset++] = point.getCoordinate(i); } - return StreamSupport.stream(ValuesAtPointIterator.create(coverage, coordinates, numPoints, ifOutside), parallel); - } catch (CannotEvaluateException ex) { - throw ex; - } catch (RuntimeException | FactoryException | TransformException ex) { - throw new CannotEvaluateException(ex.getMessage(), ex); + numPointsToTransform++; + } + /* + * Transforms the remaining sequence of points. + * Actually, in most cases, all points are transformed here. + */ + if (numPointsToTransform > 0) { // Because `toGrid` may be null. + assert toGrid.getTargetDimensions() == dimension; + toGrid.transform(coordinates, firstCoordToTransform, + coordinates, firstCoordToTransform, + numPointsToTransform); + } + final int numPoints = firstCoordToTransform / dimension + numPointsToTransform; + postTransform(coordinates, 0, numPoints); + /* + * Create the iterator. The `ValuesAtPointIterator.create(…)` method will identify the slices in + * n-dimensional coverage, get the rendered images for the regions of interest and get the tiles. + */ + final IntFunction<PointOutsideCoverageException> ifOutside; + if (nullIfOutside) { + ifOutside = null; + } else { + final var listOfPoints = (points instanceof List<?>) ? (List<? extends DirectPosition>) points : null; + ifOutside = (index) -> { + DirectPosition point = null; + if (listOfPoints != null) try { + point = listOfPoints.get(index); + } catch (IndexOutOfBoundsException e) { + recoverableException("pointOutsideCoverage", e); + } + if (point != null) { + return pointOutsideCoverage(point); + } + return new PointOutsideCoverageException(Resources.format(Resources.Keys.PointOutsideCoverageDomain_1, "#" + index)); + }; } + return ValuesAtPointIterator.create(coverage, coordinates, numPoints, ifOutside); } /** diff --git a/endorsed/src/org.apache.sis.feature/main/org/apache/sis/coverage/grid/ValuesAtPointIterator.java b/endorsed/src/org.apache.sis.feature/main/org/apache/sis/coverage/grid/ValuesAtPointIterator.java index b49dd56446..fc3aeb1c66 100644 --- a/endorsed/src/org.apache.sis.feature/main/org/apache/sis/coverage/grid/ValuesAtPointIterator.java +++ b/endorsed/src/org.apache.sis.feature/main/org/apache/sis/coverage/grid/ValuesAtPointIterator.java @@ -95,7 +95,7 @@ abstract class ValuesAtPointIterator implements Spliterator<double[]> { protected final IntFunction<PointOutsideCoverageException> ifOutside; /** - * Creates a new iterator which will traverses a subset of the given grid coordinates. + * Creates a new iterator which will traverse a subset of the given grid coordinates. * Subclasses should initialize {@link #indexOfXY} to the index of the first valid coordinate. * * @param nearestXY grid coordinates of points to evaluate, or {@code null}. @@ -199,6 +199,7 @@ abstract class ValuesAtPointIterator implements Spliterator<double[]> { * Creates a new iterator over a subset of the given iterator. * This iterator will evaluate a prefix of the sequence of points of the given iterator. * Caller should change the parent iterator to a suffix after this constructor returned. + * In particular, {@code parent.current} shall be replaced by a next instance. * This constructor is for {@link #trySplit()} implementation only. * * @param parent the iterator from which to create a prefix. @@ -281,6 +282,10 @@ abstract class ValuesAtPointIterator implements Spliterator<double[]> { * Tries to split this iterator. If successful, the returned iterator is a prefix * of the sequence of points to evaluate, and this iterator become the remaining. * + * A thread calling {@code trySplit()} may hand over the returned {@code Spliterator} to another thread, + * which in turn may further split that {@code Spliterator}. Therefore, this method must ensures that no + * instance share the same {@link #current} instance when this method returns. + * * @return an iterator covering a prefix of the points, or {@code null} if this iterator cannot be split. */ @Override @@ -293,7 +298,7 @@ abstract class ValuesAtPointIterator implements Spliterator<double[]> { i = Arrays.binarySearch(firstGridCoordOfChildren, nextChildIndex, upperChildIndex, i); if (i < 0) i = ~i; // Tild operator, not minus. It gives the insertion point. if (i > nextChildIndex && i < upperChildIndex) { - final var prefix = createPrefix(i); + final Group prefix = createPrefix(i); nextChildIndex = prefix.upperChildIndex; indexOfXY = prefix.limitOfXY; current = nextChild(); @@ -511,6 +516,8 @@ changeOfSlice: while (numPoints != 0) { /** * Creates a new iterator covering a prefix of the points. * This is invoked by {@link #trySplit()} implementation. + * Caller shall update {@link #current}, {@link #indexOfXY} and {@link #nextChildIndex} + * after this method call, because the current values are taken by the returned instance. */ @Override final Group createPrefix(int stopAtXY) { @@ -672,6 +679,8 @@ nextTile: for (tileCount = 0; indexOfXY < limitOfXY; tileCount++) { /** * Creates a new iterator covering a prefix of the points. * This is invoked by {@link #trySplit()} implementation. + * Caller shall update {@link #current}, {@link #indexOfXY} and {@link #nextChildIndex} + * after this method call, because the current values are taken by the returned instance. */ @Override final Group createPrefix(int stopAtXY) { diff --git a/endorsed/src/org.apache.sis.feature/test/org/apache/sis/coverage/grid/DefaultEvaluatorTest.java b/endorsed/src/org.apache.sis.feature/test/org/apache/sis/coverage/grid/DefaultEvaluatorTest.java index 5c3fb02c2f..26234b75a5 100644 --- a/endorsed/src/org.apache.sis.feature/test/org/apache/sis/coverage/grid/DefaultEvaluatorTest.java +++ b/endorsed/src/org.apache.sis.feature/test/org/apache/sis/coverage/grid/DefaultEvaluatorTest.java @@ -20,9 +20,12 @@ import java.util.Random; import java.util.Arrays; import java.util.List; import java.util.HashSet; +import java.util.ArrayList; +import java.util.Spliterator; import java.util.stream.Stream; import java.awt.image.DataBuffer; import javax.measure.IncommensurableException; +import org.opengis.util.FactoryException; import org.opengis.geometry.DirectPosition; import org.opengis.referencing.cs.CoordinateSystem; import org.opengis.referencing.operation.Matrix; @@ -240,6 +243,42 @@ public final class DefaultEvaluatorTest extends TestCase { return Arrays.asList(points); } + /** + * Tries to split the given iterator and adds the prefix and suffix to the given list. + */ + private static void trySplit(final Spliterator<double[]> it, final List<Spliterator<double[]>> addTo) { + final Spliterator<double[]> prefix = it.trySplit(); + if (prefix != null) { + trySplit(prefix, addTo); + trySplit(it, addTo); + } else { + addTo.add(it); + } + } + + /** + * Tests {@link ValuesAtPointIterator#trySplit()}. + * + * @throws FactoryException if the transform to a <abbr>CRS</abbr> cannot be found. + * @throws TransformException if a test point cannot be computed. + */ + @Test + public void testTrySplit() throws FactoryException, TransformException { + evaluator.setNullIfOutside(true); + @SuppressWarnings("LocalVariableHidesMemberVariable") + final var evaluator = assertInstanceOf(DefaultEvaluator.class, this.evaluator); + final var iterators = new ArrayList<Spliterator<double[]>>(); + trySplit(evaluator.spliterator(createTestPoints(true)), iterators); + final var actual = new ArrayList<double[]>(); + iterators.forEach((it) -> it.forEachRemaining((samples) -> { + if (samples != null) { + samples = samples.clone(); + } + actual.add(samples); + })); + compareSampleValues(actual.toArray(double[][]::new)); + } + /** * Compares the actual sample values against the expected values. * The given stream should compute sample values for the points @@ -256,6 +295,15 @@ public final class DefaultEvaluatorTest extends TestCase { assertTrue(isNullIfOutside, "Unexpected null array of sample values."); return null; }).toArray(double[][]::new); + compareSampleValues(actual); + } + + /** + * Compares the given values against the expected sample values. + * + * @param actual the sample values computed by the evaluator. + */ + private void compareSampleValues(final double[][] actual) { assertEquals(expectedValues.length, actual.length); for (int i=0; i<actual.length; i++) { double expected = expectedValues[i];
