jiayuasu commented on code in PR #3321:
URL: https://github.com/apache/sedona/pull/3321#discussion_r3938938931
##########
common/src/main/java/org/apache/sedona/common/raster/RasterEditors.java:
##########
@@ -710,17 +710,20 @@ public static GridCoverage2D reprojectMatch(
}
private static Interpolation createInterpolationAlgorithm(String algorithm) {
- Interpolation interp =
Interpolation.getInstance(Interpolation.INTERP_NEAREST);
- if (!Objects.isNull(algorithm) && !algorithm.isEmpty()) {
- if (algorithm.equalsIgnoreCase("nearestneighbor")) {
- interp = Interpolation.getInstance(Interpolation.INTERP_NEAREST);
- } else if (algorithm.equalsIgnoreCase("bilinear")) {
- interp = Interpolation.getInstance(Interpolation.INTERP_BILINEAR);
- } else if (algorithm.equalsIgnoreCase("bicubic")) {
- interp = Interpolation.getInstance(Interpolation.INTERP_BICUBIC);
- }
+ if (Objects.isNull(algorithm) || algorithm.isEmpty()) {
+ return Interpolation.getInstance(Interpolation.INTERP_NEAREST);
+ }
+ if (algorithm.equalsIgnoreCase("nearestneighbor")) {
+ return Interpolation.getInstance(Interpolation.INTERP_NEAREST);
+ } else if (algorithm.equalsIgnoreCase("bilinear")) {
+ return Interpolation.getInstance(Interpolation.INTERP_BILINEAR);
+ } else if (algorithm.equalsIgnoreCase("bicubic")) {
+ return Interpolation.getInstance(Interpolation.INTERP_BICUBIC);
}
- return interp;
+ throw new IllegalArgumentException(
+ "Invalid 'algorithm': '"
+ + algorithm
+ + "'. Expected one of: NearestNeighbor, Bilinear, Bicubic.");
Review Comment:
Could we update the [RS_Resample
docs](https://github.com/apache/sedona/blob/e5b03900db4d64e07c14b891a72691d05a0da092/docs/api/sql/Raster-Operators/RS_Resample.md#L34)
with this change? They currently say invalid algorithm names fall back to
`NearestNeighbor`. That should now apply only to null/empty values, with
unknown names documented as errors.
##########
common/src/main/java/org/apache/sedona/common/raster/RasterEditors.java:
##########
@@ -710,17 +710,20 @@ public static GridCoverage2D reprojectMatch(
}
private static Interpolation createInterpolationAlgorithm(String algorithm) {
- Interpolation interp =
Interpolation.getInstance(Interpolation.INTERP_NEAREST);
- if (!Objects.isNull(algorithm) && !algorithm.isEmpty()) {
- if (algorithm.equalsIgnoreCase("nearestneighbor")) {
- interp = Interpolation.getInstance(Interpolation.INTERP_NEAREST);
- } else if (algorithm.equalsIgnoreCase("bilinear")) {
- interp = Interpolation.getInstance(Interpolation.INTERP_BILINEAR);
- } else if (algorithm.equalsIgnoreCase("bicubic")) {
- interp = Interpolation.getInstance(Interpolation.INTERP_BICUBIC);
- }
+ if (Objects.isNull(algorithm) || algorithm.isEmpty()) {
+ return Interpolation.getInstance(Interpolation.INTERP_NEAREST);
+ }
+ if (algorithm.equalsIgnoreCase("nearestneighbor")) {
+ return Interpolation.getInstance(Interpolation.INTERP_NEAREST);
+ } else if (algorithm.equalsIgnoreCase("bilinear")) {
+ return Interpolation.getInstance(Interpolation.INTERP_BILINEAR);
+ } else if (algorithm.equalsIgnoreCase("bicubic")) {
+ return Interpolation.getInstance(Interpolation.INTERP_BICUBIC);
}
- return interp;
+ throw new IllegalArgumentException(
Review Comment:
Could we validate the algorithm before the `noConfigChange` return in
`resample`? This still accepts `"sinc"` when the dimensions and grid origin are
unchanged:
```java
GridCoverage2D raster =
RasterConstructors.makeEmptyRaster(1, "d", 4, 3, 0, 0, 2, -2, 0, 0, 0);
RasterEditors.resample(raster, 4, 3, 0, 0, false, "sinc"); // returns raster
RasterEditors.resample(raster, 6, 5, 0, 0, false, "sinc"); // throws
```
The same happens with unchanged scales or a matching reference raster. A
no-op test would help cover this.
--
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]
