Copilot commented on code in PR #2065:
URL: https://github.com/apache/sedona/pull/2065#discussion_r2187656947
##########
common/src/main/java/org/apache/sedona/common/raster/Rasterization.java:
##########
@@ -581,17 +581,15 @@ private static Map<Double, TreeSet<Double>>
computeScanlineIntersections(
}
} else {
double slope = (worldP2.y - worldP1.y) / (worldP2.x - worldP1.x);
- // System.out.println("slope: " + slope);
for (double y = yStart; y >= yEnd; y--) {
double xIntercept = p1X + ((p1Y - y) / slope);
- if ((xIntercept < 0) || (xIntercept >=
params.writableRaster.getWidth())) {
- continue; // skip xIntercepts outside geomExtent
- }
- if (!scanlineIntersections.containsKey(y)) {
- scanlineIntersections.put(y, new TreeSet<>());
+ double xMin = (geomExtent.getMinX() - params.upperLeftX) /
params.scaleX;
+ double xMax = (geomExtent.getMaxX() - params.upperLeftX) /
params.scaleX;
Review Comment:
`xMin` and `xMax` are recomputed on every iteration; consider hoisting them
outside the `for (double y ...)` loop to avoid redundant calculations.
##########
spark/common/src/test/scala/org/apache/sedona/sql/rasteralgebraTest.scala:
##########
@@ -1651,20 +1651,20 @@ class rasteralgebraTest extends TestBaseScala with
BeforeAndAfter with GivenWhen
"ST_GeomFromWKT('POLYGON ((236722 4204770, 243900 4204770, 243900
4197590, 221170 4197590, 236722 4204770))', 26918) as geom")
var actual =
df.selectExpr("RS_ZonalStats(raster, geom, 1, 'sum', false,
true)").first().get(0)
- assertEquals(1.0896994e7, actual)
+ assertEquals(1.0795427e7, actual)
actual =
df.selectExpr("RS_ZonalStats(raster, geom, 1, 'count', false,
false)").first().get(0)
- assertEquals(185953.0, actual)
+ assertEquals(185104.0, actual)
actual = df.selectExpr("RS_ZonalStats(raster, geom, 1, 'mean', true,
false)").first().get(0)
assertEquals(58.650240700685295, actual)
- actual = df.selectExpr("RS_ZonalStats(raster, geom, 1,
'variance')").first().get(0)
- assertEquals(8563.303492088418, actual)
+ actual = df.selectExpr(s"RS_ZonalStats(raster, geom, 1,
'variance')").first().get(0)
Review Comment:
[nitpick] The `s` string interpolator is unnecessary here since there are no
embedded expressions; you can remove the `s` prefix for consistency.
```suggestion
actual = df.selectExpr("RS_ZonalStats(raster, geom, 1,
'variance')").first().get(0)
```
##########
common/src/main/java/org/apache/sedona/common/raster/Rasterization.java:
##########
@@ -549,14 +549,14 @@ private static Map<Double, TreeSet<Double>>
computeScanlineIntersections(
// Calculate scan line limits to iterate between for each segment
// Using BigDecimal to avoid floating point errors
double yStart =
- Math.ceil(
+ Math.round(
(BigDecimal.valueOf(params.upperLeftY)
.subtract(BigDecimal.valueOf(worldP1.y))
.divide(BigDecimal.valueOf(params.scaleY),
RoundingMode.CEILING))
.doubleValue());
Review Comment:
[nitpick] `yStart` and `yEnd` represent integer scanline indices; consider
using an integer type (`int` or `long`) to avoid confusion around
floating-point precision.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
