Copilot commented on code in PR #3108:
URL: https://github.com/apache/sedona/pull/3108#discussion_r3649367979
##########
python/sedona/spark/raster/sample_model.py:
##########
@@ -188,27 +189,21 @@ def __init__(
self.data_bit_offset = data_bit_offset
def as_numpy(self, data_buffer: DataBuffer) -> np.ndarray:
- bank_data = data_buffer.bank_data[0]
- bits_per_value = bank_data.dtype.itemsize * 8
- pixel_per_value = bits_per_value / self.num_bits
- shift_right = bits_per_value - self.num_bits
- mask = ((1 << self.num_bits) - 1) << shift_right
-
- band_data = []
- for y in range(self.height):
- pos = y * self.scanline_stride + self.data_bit_offset //
bits_per_value
- value = bank_data[pos]
- shift = self.data_bit_offset % bits_per_value
- value = value << shift
- pixels: List[int] = []
- while len(pixels) < self.width:
- while shift < bits_per_value and len(pixels) < self.width:
- pixels.append((value & mask) >> shift_right)
- value = value << self.num_bits
- shift += self.num_bits
- pos += 1
- value = bank_data[pos]
- shift = 0
- band_data.append(np.array(pixels, dtype=bank_data.dtype))
-
- return np.array(band_data).reshape(1, self.height, self.width)
+ samples = data_buffer.bank_samples()
+ bits_per_value = samples.dtype.itemsize * 8
+
+ # Resolve every pixel on its own, the way Java does: this sample model
requires
+ # num_bits to divide the size of a data element, so no pixel spans two
elements.
+ pixel_bits = self.data_bit_offset + np.arange(self.width) *
self.num_bits
+ cols = pixel_bits // bits_per_value
+ shifts = bits_per_value - (pixel_bits % bits_per_value) - self.num_bits
Review Comment:
MultiPixelPackedSampleModel.as_numpy currently assumes pixels never span two
data elements (see the comment) and extracts each pixel from a single element
via a simple shift/mask. When data_bit_offset is not a multiple of num_bits
(e.g., 4-bit pixels with data_bit_offset=2), Java AWT will read pixels that
span two elements; the current vectorized formula will return incorrect values
for those columns.
##########
python/tests/raster/test_sample_model.py:
##########
@@ -0,0 +1,277 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+"""Tests for reading rasters whose sample models use the less common layouts.
+
+The expected samples of every test were produced by handing the same sample
model and data
+buffer to java.awt.image, so they record what Java AWT reads for these
layouts. Each test
+names the Java layout it covers, which is enough to reproduce it.
+"""
+
+from typing import List
+
+import numpy as np
+import pytest
+
+from sedona.spark.raster.data_buffer import DataBuffer
+from sedona.spark.raster.sample_model import (
+ ComponentSampleModel,
+ MultiPixelPackedSampleModel,
+ PixelInterleavedSampleModel,
+ SinglePixelPackedSampleModel,
+)
+
+
+def ramp(start: int, length: int, dtype=np.int32) -> np.ndarray:
+ """A bank of consecutive values, so that a sample identifies its own
position."""
+ return np.arange(start, start + length, dtype=dtype)
+
+
+def signed_int32(mask: int) -> int:
+ """Turn a bit mask written in its unsigned form into the value the
deserializer
+ produces for it, which is negative for masks covering the top bit."""
+ return mask - (1 << 32) if mask >= (1 << 31) else mask
+
+
+def int_bank(values: List[int]) -> np.ndarray:
+ """An int32 bank holding values written in their unsigned form."""
+ return np.array(values, dtype=np.uint32).astype(np.int32)
+
+
+def test_component_fast_path_pairs_band_offsets_with_bands() -> None:
+ # ComponentSampleModel(TYPE_INT, 2, 2, 1, 2, {1, 0}, {3, 1})
+ sample_model = ComponentSampleModel(DataBuffer.TYPE_INT, 2, 2, 1, 2, [1,
0], [3, 1])
+ data_buffer = DataBuffer(DataBuffer.TYPE_INT, [ramp(10, 7), ramp(20, 7)],
7, [0, 0])
+
+ np.testing.assert_array_equal(
+ sample_model.as_numpy(data_buffer),
+ np.array([[[23, 24], [25, 26]], [[11, 12], [13, 14]]]),
+ )
+
+
+def test_component_fast_path_applies_bank_offsets() -> None:
+ # ComponentSampleModel(TYPE_INT, 2, 2, 1, 2, {1, 0}, {3, 1})
+ # over DataBufferInt(banks, 7, {2, 1})
+ sample_model = ComponentSampleModel(DataBuffer.TYPE_INT, 2, 2, 1, 2, [1,
0], [3, 1])
+ data_buffer = DataBuffer(DataBuffer.TYPE_INT, [ramp(10, 9), ramp(20, 9)],
7, [2, 1])
+
+ np.testing.assert_array_equal(
+ sample_model.as_numpy(data_buffer),
+ np.array([[[24, 25], [26, 27]], [[13, 14], [15, 16]]]),
+ )
+
+
+def test_component_fast_path_reads_a_bounded_window_of_padded_banks() -> None:
+ # ComponentSampleModel(TYPE_INT, 2, 2, 1, 2, {0, 1}, {0, 0}) over banks of
6 samples,
+ # so the band offsets alone give no reason to narrow a bank down to 4
samples
+ sample_model = ComponentSampleModel(DataBuffer.TYPE_INT, 2, 2, 1, 2, [0,
1], [0, 0])
+ data_buffer = DataBuffer(DataBuffer.TYPE_INT, [ramp(10, 6), ramp(20, 6)],
6, [0, 0])
+
+ np.testing.assert_array_equal(
+ sample_model.as_numpy(data_buffer),
+ np.array([[[10, 11], [12, 13]], [[20, 21], [22, 23]]]),
+ )
+
+
+def test_banded_sample_model_over_reordered_banks() -> None:
+ # BandedSampleModel(TYPE_INT, 2, 2, 2, {1, 0}, {1, 2}) over
DataBufferInt(banks, 6, {1, 2}),
+ # which is deserialized as a component model with a unit pixel stride
+ sample_model = ComponentSampleModel(DataBuffer.TYPE_INT, 2, 2, 1, 2, [1,
0], [1, 2])
+ data_buffer = DataBuffer(DataBuffer.TYPE_INT, [ramp(10, 8), ramp(20, 8)],
6, [1, 2])
+
+ np.testing.assert_array_equal(
+ sample_model.as_numpy(data_buffer),
+ np.array([[[23, 24], [25, 26]], [[13, 14], [15, 16]]]),
+ )
+
+
+def test_component_slow_path_applies_bank_offsets() -> None:
+ # ComponentSampleModel(TYPE_INT, 3, 2, 2, 8, {1, 1, 0}, {0, 1, 4})
+ # over DataBufferInt(banks, 20, {3, 2}): two bands share a bank, and there
are gaps
+ # both between the scanlines and between the pixels within them
+ sample_model = ComponentSampleModel(
+ DataBuffer.TYPE_INT, 3, 2, 2, 8, [1, 1, 0], [0, 1, 4]
+ )
+ data_buffer = DataBuffer(
+ DataBuffer.TYPE_INT, [ramp(100, 24), ramp(200, 24)], 20, [3, 2]
+ )
+
+ np.testing.assert_array_equal(
+ sample_model.as_numpy(data_buffer),
+ np.array(
+ [
+ [[202, 204, 206], [210, 212, 214]],
+ [[203, 205, 207], [211, 213, 215]],
+ [[107, 109, 111], [115, 117, 119]],
+ ]
+ ),
+ )
+
+
+def test_pixel_interleaved_fast_path_applies_bank_offset() -> None:
+ # PixelInterleavedSampleModel(TYPE_INT, 2, 2, 3, 6, {0, 1, 2})
+ # over DataBufferInt(bank of 20 samples, 17, {3})
+ sample_model = PixelInterleavedSampleModel(
+ DataBuffer.TYPE_INT, 2, 2, 3, 6, [0, 1, 2]
+ )
+ data_buffer = DataBuffer(DataBuffer.TYPE_INT, [ramp(10, 20)], 17, [3])
+
+ np.testing.assert_array_equal(
+ sample_model.as_numpy(data_buffer),
+ np.array([[[13, 16], [19, 22]], [[14, 17], [20, 23]], [[15, 18], [21,
24]]]),
+ )
+
+
+def test_pixel_interleaved_slow_path_band_offsets_may_exceed_pixel_stride() ->
None:
+ # PixelInterleavedSampleModel(TYPE_INT, 2, 2, 4, 10, {3, 1, 0})
+ # over DataBufferInt(bank, 20, {2}): band 0 sits past the window the three
bands of a
+ # single pixel would occupy
+ sample_model = PixelInterleavedSampleModel(
+ DataBuffer.TYPE_INT, 2, 2, 4, 10, [3, 1, 0]
+ )
+ data_buffer = DataBuffer(DataBuffer.TYPE_INT, [ramp(10, 24)], 20, [2])
+
+ np.testing.assert_array_equal(
+ sample_model.as_numpy(data_buffer),
+ np.array([[[15, 19], [25, 29]], [[13, 17], [23, 27]], [[12, 16], [22,
26]]]),
+ )
+
+
+def test_single_pixel_packed_argb_does_not_sign_extend() -> None:
+ # SinglePixelPackedSampleModel(TYPE_INT, 2, 2, 3, {0xFF000000, 0xFF0000,
0xFF00, 0xFF})
+ # over DataBufferInt(bank, 7, {1}): the alpha mask covers the sign bit of
the samples
+ sample_model = SinglePixelPackedSampleModel(
+ DataBuffer.TYPE_INT,
+ 2,
+ 2,
+ 3,
+ [signed_int32(0xFF000000), 0x00FF0000, 0x0000FF00, 0x000000FF],
+ )
+ data_buffer = DataBuffer(
+ DataBuffer.TYPE_INT,
+ [int_bank([0, 0x8090A0B0, 0x102030F0, 0, 0xFFFFFFFF, 0x01020304, 0,
0])],
+ 7,
+ [1],
+ )
+
+ np.testing.assert_array_equal(
+ sample_model.as_numpy(data_buffer),
+ np.array(
+ [
+ [[128, 16], [255, 1]],
+ [[144, 32], [255, 2]],
+ [[160, 48], [255, 3]],
+ [[176, 240], [255, 4]],
+ ]
+ ),
+ )
+
+
+def test_single_pixel_packed_applies_bank_offset() -> None:
+ # SinglePixelPackedSampleModel(TYPE_USHORT, 2, 2, 4, {0xF800, 0x7E0,
0x1F}) over
+ # DataBufferUShort(bank, 8, {2}): RGB 565 pixels starting two samples into
the bank
+ sample_model = SinglePixelPackedSampleModel(
+ DataBuffer.TYPE_USHORT, 2, 2, 4, [0xF800, 0x07E0, 0x001F]
+ )
+ data_buffer = DataBuffer(
+ DataBuffer.TYPE_USHORT,
+ [np.array([0, 0xF81F, 0x07E0, 0, 0, 0x1234, 0xFFFF, 0],
dtype=np.uint16)],
+ 8,
+ [2],
+ )
+
+ np.testing.assert_array_equal(
+ sample_model.as_numpy(data_buffer),
+ np.array([[[0, 0], [31, 0]], [[63, 0], [63, 0]], [[0, 0], [31, 0]]]),
+ )
+
+
+def test_multi_pixel_packed_applies_bit_and_bank_offsets() -> None:
+ # MultiPixelPackedSampleModel(TYPE_BYTE, 5, 2, 4, 4, 4) over
DataBufferByte(bank, 9, {1}):
+ # four bits per pixel, with both the bank and the first scanline starting
half a byte in
+ sample_model = MultiPixelPackedSampleModel(DataBuffer.TYPE_BYTE, 5, 2, 4,
4, 4)
+ data_buffer = DataBuffer(
+ DataBuffer.TYPE_BYTE,
+ [
+ np.array(
+ [0xFF, 0x01, 0x23, 0x45, 0x67, 0x89, 0xAB, 0xCD, 0xEF, 0x00],
+ dtype=np.uint8,
+ )
+ ],
+ 9,
+ [1],
+ )
+
+ np.testing.assert_array_equal(
+ sample_model.as_numpy(data_buffer),
+ np.array([[[1, 2, 3, 4, 5], [9, 10, 11, 12, 13]]]),
+ )
+
+
+def test_multi_pixel_packed_stays_within_the_bank() -> None:
Review Comment:
The new MultiPixelPackedSampleModel coverage only exercises data_bit_offset
values that are multiples of num_bits (so each pixel fits within one element).
A regression in the spanning-two-elements case (e.g., num_bits=4,
data_bit_offset=2) would not be caught; adding a small unaligned-offset test
would lock in the intended Java AWT behavior.
--
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]
