james-willis commented on code in PR #749:
URL: https://github.com/apache/sedona-db/pull/749#discussion_r3112936904
##########
rust/sedona-raster/src/affine_transformation.rs:
##########
@@ -124,139 +132,123 @@ pub fn to_raster_coordinate(
world_y: f64,
) -> Result<(i64, i64), ArrowError> {
let (rx, ry) =
- AffineMatrix::from_metadata(raster.metadata()).inv_transform(world_x,
world_y)?;
+
AffineMatrix::from_transform(raster.transform()).inv_transform(world_x,
world_y)?;
Ok((rx as i64, ry as i64))
}
#[cfg(test)]
mod tests {
use super::*;
- use crate::traits::{MetadataRef, RasterMetadata};
use approx::assert_relative_eq;
use std::f64::consts::FRAC_1_SQRT_2;
use std::f64::consts::PI;
+ /// Minimal RasterRef implementation for testing affine transforms.
struct TestRaster {
- metadata: RasterMetadata,
+ transform: [f64; 6],
+ }
+
+ impl TestRaster {
+ fn new(
+ origin_x: f64,
+ origin_y: f64,
+ scale_x: f64,
+ scale_y: f64,
+ skew_x: f64,
+ skew_y: f64,
+ ) -> Self {
+ Self {
+ transform: [origin_x, scale_x, skew_x, origin_y, skew_y,
scale_y],
+ }
+ }
}
impl RasterRef for TestRaster {
- fn metadata(&self) -> &dyn MetadataRef {
- &self.metadata
+ fn num_bands(&self) -> usize {
+ 0
+ }
+ fn band(&self, _index: usize) -> Option<Box<dyn crate::traits::BandRef
+ '_>> {
+ None
+ }
+ fn band_name(&self, _index: usize) -> Option<&str> {
+ None
}
fn crs(&self) -> Option<&str> {
None
}
- fn bands(&self) -> &dyn crate::traits::BandsRef {
- unimplemented!()
+ fn transform(&self) -> &[f64] {
+ &self.transform
+ }
+ fn x_dim(&self) -> &str {
+ "x"
+ }
+ fn y_dim(&self) -> &str {
+ "y"
}
}
#[test]
fn test_rotation() {
- // 0 degree rotation -> gt[1.0, 0.0, 0.0, -1.0]
- let raster = rotation_raster(1.0, -1.0, 0.0, 0.0);
- let rot = rotation(&raster);
- assert_eq!(rot, 0.0);
-
- // pi/2 -> gt[0.0, -1.0, 1.0, 0.0]
- let raster = rotation_raster(0.0, 0.0, -1.0, 1.0);
- let rot = rotation(&raster);
- assert_relative_eq!(rot, PI / 2.0, epsilon = 1e-6); // 90 degrees in
radians
-
- // pi/4 -> gt[0.70710678, -0.70710678, 0.70710678, 0.70710678]
- let raster = rotation_raster(FRAC_1_SQRT_2, FRAC_1_SQRT_2,
-FRAC_1_SQRT_2, FRAC_1_SQRT_2);
- let rot = rotation(&raster);
- assert_relative_eq!(rot, PI / 4.0, epsilon = 1e-6); // 45 degrees in
radians
-
- // pi/3 -> gt[0.5, -0.866025, 0.866025, 0.5]
- let raster = rotation_raster(0.5, 0.5, -0.866025, 0.866025);
- let rot = rotation(&raster);
- assert_relative_eq!(rot, PI / 3.0, epsilon = 1e-6); // 60 degrees in
radians
-
- // pi -> gt[-1.0, 0.0, 0.0, -1.0]
- let raster = rotation_raster(-1.0, -1.0, 0.0, 0.0);
- let rot = rotation(&raster);
- assert_relative_eq!(rot, -PI, epsilon = 1e-6); // 180 degrees in
radians
+ // 0 degree rotation
+ let raster = TestRaster::new(0.0, 0.0, 1.0, -1.0, 0.0, 0.0);
+ assert_eq!(rotation(&raster), 0.0);
+
+ // pi/2
+ let raster = TestRaster::new(0.0, 0.0, 0.0, 0.0, -1.0, 1.0);
+ assert_relative_eq!(rotation(&raster), PI / 2.0, epsilon = 1e-6);
+
+ // pi/4
+ let raster = TestRaster::new(
+ 0.0,
+ 0.0,
+ FRAC_1_SQRT_2,
+ FRAC_1_SQRT_2,
+ -FRAC_1_SQRT_2,
+ FRAC_1_SQRT_2,
+ );
+ assert_relative_eq!(rotation(&raster), PI / 4.0, epsilon = 1e-6);
+
+ // pi/3
+ let raster = TestRaster::new(0.0, 0.0, 0.5, 0.5, -0.866025, 0.866025);
+ assert_relative_eq!(rotation(&raster), PI / 3.0, epsilon = 1e-6);
+
+ // pi
+ let raster = TestRaster::new(0.0, 0.0, -1.0, -1.0, 0.0, 0.0);
+ assert_relative_eq!(rotation(&raster), -PI, epsilon = 1e-6);
}
#[test]
fn test_to_world_coordinate() {
- // Test case with rotation/skew
- let raster = TestRaster {
- metadata: RasterMetadata {
- width: 10,
- height: 20,
- upperleft_x: 100.0,
- upperleft_y: 200.0,
- scale_x: 1.0,
- scale_y: -2.0,
- skew_x: 0.25,
- skew_y: 0.5,
- },
- };
-
- let (wx, wy) = to_world_coordinate(&raster, 0, 0);
- assert_eq!((wx, wy), (100.0, 200.0));
-
- let (wx, wy) = to_world_coordinate(&raster, 5, 10);
- assert_eq!((wx, wy), (107.5, 182.5));
+ let raster = TestRaster::new(100.0, 200.0, 1.0, -2.0, 0.25, 0.5);
- let (wx, wy) = to_world_coordinate(&raster, 9, 19);
- assert_eq!((wx, wy), (113.75, 166.5));
-
- let (wx, wy) = to_world_coordinate(&raster, 1, 0);
- assert_eq!((wx, wy), (101.0, 200.5));
-
- let (wx, wy) = to_world_coordinate(&raster, 0, 1);
- assert_eq!((wx, wy), (100.25, 198.0));
+ assert_eq!(to_world_coordinate(&raster, 0, 0), (100.0, 200.0));
+ assert_eq!(to_world_coordinate(&raster, 5, 10), (107.5, 182.5));
+ assert_eq!(to_world_coordinate(&raster, 9, 19), (113.75, 166.5));
+ assert_eq!(to_world_coordinate(&raster, 1, 0), (101.0, 200.5));
+ assert_eq!(to_world_coordinate(&raster, 0, 1), (100.25, 198.0));
}
#[test]
fn test_to_raster_coordinate() {
- // Test case with rotation/skew
- let raster = TestRaster {
- metadata: RasterMetadata {
- width: 10,
- height: 20,
- upperleft_x: 100.0,
- upperleft_y: 200.0,
- scale_x: 1.0,
- scale_y: -2.0,
- skew_x: 0.25,
- skew_y: 0.5,
- },
- };
-
- // Reverse of the to_world_coordinate tests
- let (wx, wy) = to_raster_coordinate(&raster, 100.0, 200.0).unwrap();
- assert_eq!((wx, wy), (0, 0));
-
- let (wx, wy) = to_raster_coordinate(&raster, 107.5, 182.5).unwrap();
- assert_eq!((wx, wy), (5, 10));
-
- let (wx, wy) = to_raster_coordinate(&raster, 113.75, 166.5).unwrap();
- assert_eq!((wx, wy), (9, 19));
-
- let (wx, wy) = to_raster_coordinate(&raster, 101.0, 200.5).unwrap();
- assert_eq!((wx, wy), (1, 0));
-
- let (wx, wy) = to_raster_coordinate(&raster, 100.25, 198.0).unwrap();
- assert_eq!((wx, wy), (0, 1));
-
- // Check error handling for zero determinant
- let bad_raster = TestRaster {
- metadata: RasterMetadata {
- width: 10,
- height: 20,
- upperleft_x: 100.0,
- upperleft_y: 200.0,
- scale_x: 1.0,
- scale_y: 0.0,
- skew_x: 0.0,
- skew_y: 0.0,
- },
- };
+ let raster = TestRaster::new(100.0, 200.0, 1.0, -2.0, 0.25, 0.5);
Review Comment:
Fixed — replaced with a `test_raster(origin_x, origin_y, scale_x, scale_y,
skew_x, skew_y)` free function so the parameter names are visible at the call
site.
##########
rust/sedona-raster/src/affine_transformation.rs:
##########
@@ -324,11 +301,6 @@ mod tests {
};
let result = a.inv_transform(0.0, 0.0);
assert!(result.is_err());
- assert!(result
- .err()
- .unwrap()
- .to_string()
- .contains("determinant is zero."));
Review Comment:
Confirmed — the expectation is preserved. Same test logic, just uses the new
constructor.
##########
rust/sedona-raster/src/display.rs:
##########
@@ -113,51 +93,3 @@ impl fmt::Display for RasterDisplay<'_> {
Ok(())
}
}
-
-#[cfg(test)]
-mod tests {
- use super::*;
- use crate::array::RasterStructArray;
- use sedona_testing::rasters::generate_test_rasters;
-
- #[test]
- fn display_non_skewed_raster() {
Review Comment:
Re-added all three tests and the doc comments.
##########
rust/sedona-testing/src/rasters.rs:
##########
@@ -292,8 +215,6 @@ pub fn generate_multi_band_raster() -> StructArray {
builder.finish().unwrap()
}
-/// Determine if this tile contains a corner of the overall grid and return
its position
-/// Returns Some(position) if this tile contains a corner, None otherwise
Review Comment:
No good reason — accidentally dropped during the rewrite. Restored.
##########
rust/sedona-testing/src/rasters.rs:
##########
@@ -578,65 +492,33 @@ mod tests {
fn test_raster_equal() {
let raster_array1 =
generate_tiled_rasters((256, 256), (1, 1), BandDataType::UInt8,
Some(43)).unwrap();
- let raster1 = RasterStructArray::new(&raster_array1).get(0).unwrap();
-
- // Assert that the rasters are equal to themselves
+ let rsa = RasterStructArray::new(&raster_array1);
+ let raster1 = rsa.get(0).unwrap();
assert_raster_equal(&raster1, &raster1);
}
#[test]
- #[should_panic = "Band data does not match"]
+ #[should_panic = "Band 0 data does not match"]
fn test_raster_different_band_data() {
let raster_array1 =
generate_tiled_rasters((128, 128), (1, 1), BandDataType::UInt8,
Some(43)).unwrap();
let raster_array2 =
generate_tiled_rasters((128, 128), (1, 1), BandDataType::UInt8,
Some(47)).unwrap();
-
- let raster1 = RasterStructArray::new(&raster_array1).get(0).unwrap();
- let raster2 = RasterStructArray::new(&raster_array2).get(0).unwrap();
+ let rsa1 = RasterStructArray::new(&raster_array1);
+ let rsa2 = RasterStructArray::new(&raster_array2);
+ let raster1 = rsa1.get(0).unwrap();
+ let raster2 = rsa2.get(0).unwrap();
assert_raster_equal(&raster1, &raster2);
}
#[test]
- fn test_generate_multi_band_raster() {
- let struct_array = generate_multi_band_raster();
- let raster_array = RasterStructArray::new(&struct_array);
- assert_eq!(raster_array.len(), 1);
Review Comment:
Still relevant — the equivalent test is `test_raster_different_metadata`
which now panics with "Raster transforms do not match" instead of "Raster upper
left x does not match" (same data, different field name in the assertion).
--
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]
