[GH-3166] Resolve raster sample positions the way Java AWT does in the Python reader - #3108
Conversation
jiayuasu
left a comment
jiayuasu
left a comment
There was a problem hiding this comment.
One question on the offset handling.
| for k, bank_index in enumerate(self.bank_indices): | ||
| bank_data = data_buffer.bank_data[bank_index] | ||
| offset = self.band_offsets[bank_index] | ||
| offset = self.band_offsets[k] |
There was a problem hiding this comment.
Could we calculate the effective start here (data_buffer.offsets[bank_index] + self.band_offsets[k]) and always slice a width * height window? I may be missing an invariant, but a component model can have a zero band offset with a padded backing bank; then the current branch seems to pass the whole bank to reshape(). The slow path may need the buffer offset too. A zero-offset case in the new test would help confirm it.
The band offset mix-up in ComponentSampleModel's fast path was one of several places where the python sample models do not resolve sample positions the way java.awt.image does: - None of the four sample models applied the offsets of the data buffer, so every sample of a bank with a non-zero offset was read from the wrong position. DataBuffer.bank_samples() now applies them in one place, the way DataBuffer.getElem() does in Java. - The two fast paths reshaped a whole bank instead of a bounded window, so a bank holding more samples than the image needs, which is what a non-zero buffer offset implies, failed to reshape at all. - PixelInterleavedSampleModel's slow path looked band offsets up within one pixel stride, but they are positions within a scanline and may reach past it. - SinglePixelPackedSampleModel masked and shifted signed samples, so a mask covering the sign bit, such as the alpha mask of an ARGB raster, produced sign-extended band values instead of Java's `>>>` result. - MultiPixelPackedSampleModel always read one sample past the end of a scanline, which is out of bounds when the last scanline ends at the end of the bank, and its running bit shift diverged from Java for data bit offsets that are not a multiple of num_bits. Both packed models now resolve each pixel on its own, as Java does, and the strided paths index with numpy instead of looping over every pixel in python. The expected samples of the new tests were taken from java.awt.image, given the same sample models and data buffers.
jiayuasu
changed the title
There was a problem hiding this comment.
Pull request overview
This PR aligns the Python raster SampleModel implementations with Java AWT (java.awt.image) for less-common sample layouts, fixing multiple sample-positioning and bit-extraction discrepancies so SedonaRaster.as_numpy() matches Raster.getSample(...).
Changes:
- Apply
DataBuffer.offsetsconsistently via a newDataBuffer.bank_samples()accessor and update sample models to index the offset-adjusted arrays. - Fix/replace fast and slow paths in component and pixel-interleaved models to use correct band/bank indexing and bounded windows.
- Update packed sample models to use unsigned shifting/masking semantics consistent with Java AWT, and add targeted unit tests.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| python/sedona/spark/raster/sample_model.py | Refactors sample extraction logic across sample models to match Java AWT positioning/bit semantics; adds shared pixel-position helper. |
| python/sedona/spark/raster/data_buffer.py | Introduces bank_samples() to apply per-bank offsets centrally (Java getElem semantics). |
| python/tests/raster/test_sample_model.py | Adds Java-derived expected-value tests for edge-case layouts across the sample models. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # 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 |
|
|
||
|
|
||
| def test_multi_pixel_packed_stays_within_the_bank() -> None: |
Two more places where the packed sample models read something other than what java.awt.image reads: - MultiPixelPackedSampleModel derived its bit mask as `(1 << num_bits) - 1` with python integers. Java derives it on an int, whose shift count is taken modulo 32, so a pixel occupying a whole 32 bit sample gets a zero mask there and Java reads every pixel of such a raster as zero, while the port returned the backing words. - SinglePixelPackedSampleModel derived a bit offset of -1 for a zero bit mask, which Java accepts and reads as a zero band. Shifting by -1 read the band as zero by accident on numpy 1.x and raised OverflowError on numpy 2.x. Swept both packed models over 1004 layouts recorded from java.awt.image, all valid num_bits for byte/ushort/int samples, aligned and unaligned data bit offsets, non-zero bank offsets, and mask sets covering zero masks, whole sample masks and masks over the sign bit. All 1004 now match, on numpy 1.26 and 2.5; 510 of them do not match before this branch.
|
Confirmed against the JDK and fixed in 642de29. [P1] MultiPixelPackedSampleModel(TYPE_INT, 3, 2, 32, 4, 0) is accepted by Java and reads every pixel as zero, because bitMask = (1 << 32) - 1 is evaluated on an int whose shift count is taken modulo 32. The mask is now derived as (1 << (num_bits % 32)) - 1, which reproduces that and leaves every other width alone (8 on byte and 16 on ushort still mask normally). [P2] Java leaves the bit offset of a zero mask at 0 and reads the band as zero; getBitOffsets() returns [16, 8, 0, 0] for {0xFF0000, 0xFF00, 0xFF, 0}. The offset is now 0 for a zero mask. Worth noting the crash was numpy-2 only: on numpy 1.26 the -1 shift happened to yield the right answer, so this would not have shown up in CI. Rather than patch just these two, I swept both packed models over 1004 layouts recorded from java.awt.image: every num_bits that divides the sample size for byte, ushort and int samples, aligned and unaligned data bit offsets, zero and non-zero bank offsets, and mask sets covering zero masks, whole-sample masks and masks over the sign bit. All 1004 match on numpy 1.26 and 2.5, with numpy warnings raised as errors; 510 of them do not match on master. No third case of this kind is left. One thing the sweep turned up: Java rejects non-contiguous bit masks in the SinglePixelPackedSampleModel constructor (Mask 195 must be contiguous), so those layouts cannot reach the reader and need no handling. Two new regression tests cover P1 and P2 (test_multi_pixel_packed_whole_sample_pixels_read_zero, test_single_pixel_packed_zero_mask_reads_zero), taking their expected values from the same JDK run. |
MultiPixelPackedSampleModel shifted the samples as unsigned values by the
distance the sample model implies. Java shifts the sample as the signed int that
DataBuffer.getElem() returns, with `>>`, and takes the distance of an int shift
modulo 32. A data bit offset that is not a multiple of num_bits leaves a pixel
straddling two samples and makes that distance negative, where Java then shifts
the top bits of the sample down while numpy shifted the whole sample out:
MultiPixelPackedSampleModel(TYPE_INT, 4, 1, 8, 1, 4) over
{0x89ABCDEF, 0x12345678} reads [154, 188, 222, 248] in Java, and the last
pixel came out as 0 here. The distance for it is 32 - 28 - 8 = -4, which Java
masks to 28.
It is not only about the sign bit: the same layout over {0x12345678, 0} reads
[35, 69, 103, 1] in Java and the last pixel was 0 here as well.
The sweep that was meant to cover this only used data bit offsets that are
multiples of num_bits, so it never built a straddling pixel. It now enumerates
every offset up to two samples wide, 7580 layouts recorded from java.awt.image,
all of which match; 1291 of them did not match before this commit.
|
Confirmed and fixed in 9553cc6. JDK 17 returns Correction to my previous comment first: the claim that the 1004-case sweep covered unaligned data bit offsets was wrong. Its offsets were Root cause is narrower than the shift distance alone. Java reads a sample through It is not only about the sign bit, so I added both variants as regression cases: the same layout over Keeping the offsets in scope rather than narrowing the claim, since the reader has to handle whatever a serialized Java raster contains. The sweep now enumerates every
Verified on numpy 1.26.4 and 2.5.1 with numpy warnings raised as errors. The unit tests are at 22 cases, 21 of which fail without this branch. |
The two layouts where MultiPixelPackedSampleModel reproduces a quirk of Java's
int arithmetic, rather than reading what the sample model nominally describes,
now say so. Neither can hold a pixel that survives a round trip through Java
itself, so the values stay as Java reads them and a UserWarning names the
layout:
- a pixel occupying a whole 32 bit sample, which Java gives a zero bit mask and
reads as zero, and which Java's setSample cannot write to at all
- a pixel straddling two samples, which Java reads by shifting the top bits of
the sample down, and which Java's setSample truncates to the bits that fit
The check is the negative shift distance itself rather than the alignment of the
data bit offset, since whether a pixel straddles also depends on the width.
Reading the packed layout RS_MakeRasterForTesting builds raises nothing under
`warnings.simplefilter("error")`, and repeated reads of one odd raster warn once
per process rather than once per read.
|
Added the warning in 3fce157.
The trigger is the negative shift distance itself, not the alignment of the data bit offset, because whether a pixel straddles also depends on the width: 4 bit pixels at offset 2 in a 32 bit sample fit until x reaches 8. Checking the distance keeps it exact in both directions. Behaviour, all three verified rather than assumed:
Worth knowing where this does and does not help: inside a pandas UDF the warning lands on executor stderr, which is easy to miss. It is aimed at driver-side reads and notebooks, so it is a signpost for whoever is debugging an odd raster interactively rather than a guard rail on a cluster. |
Did you read the Contributor Guide?
Is this PR related to a ticket?
[GH-XXX] my subject. Closes Python raster reader does not resolve sample positions the way Java AWT does #3166What changes were proposed in this PR?
The Python sample models did not resolve sample positions the way
java.awt.imagedoes. The band offset mix-up inComponentSampleModel's fast path was one of several defects of that kind, all of which are fixed here:Band offsets were indexed by bank instead of by band in
ComponentSampleModel's fast path, so a raster whose bank mapping is not the identity returned samples from the wrong positions. The slow path already used the band position.The offsets of the
DataBufferwere never applied, by any of the four sample models.DataBuffer.getElem(bank, i)resolves tobankdata[bank][i + offsets[bank]]in Java, while the Python code readbankdata[bank][i], so every sample of a bank with a non-zero offset came from the wrong position.DataBuffer.bank_samples()now applies the offset in one place, and the sample models index the array it returns.The two fast paths reshaped a whole bank instead of a bounded
width * heightwindow. A bank holding more samples than the image needs, which is what a non-zero buffer offset implies, failed withcannot reshape array of size N into shape (h,w).PixelInterleavedSampleModel's slow path looked band offsets up within a single pixel stride (bank_data[begin:begin + num_bands][band_offsets]). Band offsets are positions within a scanline and may reach past the pixel they belong to, which raisedIndexErroron such layouts.SinglePixelPackedSampleModelmasked and shifted signed samples. Java extracts bands with>>>, so a mask covering the sign bit — the alpha mask of an ARGB raster,0xFF000000— produced sign-extended, negative band values instead of0..255.MultiPixelPackedSampleModelalways read one sample past the end of a scanline, which is out of bounds when the last scanline ends at the end of the bank. Its running bit shift also diverged from Java for adata_bit_offsetthat is not a multiple ofnum_bits, the case where a pixel spans two data elements; for offsets that are a multiple ofnum_bitsthe old loop and this one agree, checked over 1323 parameter combinations.MultiPixelPackedSampleModelshifted samples as unsigned values, by the distance the sample model implies. Java shifts the sample as the signedintthatDataBuffer.getElem()returns, using>>, and takes the distance of anintshift modulo 32. Two consequences were missed: adata_bit_offsetthat is not a multiple ofnum_bitsleaves a pixel straddling two samples and makes that distance negative, where Java shifts the sample's top bits down (MultiPixelPackedSampleModel(TYPE_INT, 4, 1, 8, 1, 4)over{0x89ABCDEF, 0x12345678}reads[154, 188, 222, 248], and the last pixel came out as0); and the mask, derived as(1 << numberOfBits) - 1on anint, is zero for a pixel occupying a whole 32 bit sample, so Java reads every pixel of such a raster as zero while the port returned the backing words.SinglePixelPackedSampleModelderived a bit offset of-1for a zero bit mask, which Java accepts and reads as a zero band. Shifting by-1read the band as zero by accident on numpy 1.x and raisedOverflowError: Python integer -1 out of bounds for uint32on numpy 2.x.Both packed models now resolve each pixel on its own, the way Java does, and the strided paths index with numpy instead of looping over every pixel in Python.
The two layouts behind defect 7 are quirks of Java's
intarithmetic rather than sample addressing, and neither can hold a pixel that survives a round trip through Java itself —setSampleis a no-op for a 32 bit pixel, and it truncates a straddling pixel to the bits that fit. Reading them any other way would putas_numpy()at odds withRS_Value,RS_AsGeoTiffand everything else that reads throughRaster.getSamples, without recovering the value the caller wrote, so they are reproduced as Java reads them and aUserWarningnames the layout when one is encountered.How was this patch tested?
New unit tests in
python/tests/raster/test_sample_model.py: 22 cases across all four sample models. Every expected value was produced by handing the same sample model and data buffer tojava.awt.imageand reading the samples back withRaster.getSample, so the tests record what Java AWT reads rather than what the implementation happens to produce. 21 of the 22 fail without this change.The warning is covered too: it fires for both quirk layouts, and reading the packed layout
RS_MakeRasterForTestingbuilds today raises nothing underwarnings.simplefilter("error").Both packed models were additionally swept over 7580 layouts recorded from
java.awt.image: everynum_bitsthat divides the sample size for byte, ushort and int samples, everydata_bit_offsetfrom 0 to two samples wide so that pixels straddling two samples are covered, zero and non-zero bank offsets, and mask sets covering zero masks, whole-sample masks and masks over the sign bit. All 7580 match, under numpy 1.26 and 2.5 with numpy warnings raised as errors. 5295 of them do not match on master.The layouts
RS_MakeRasterForTestingbuilds today (BandedSampleModel,PixelInterleavedSampleModel,PixelInterleavedSampleModelComplex,ComponentSampleModel,SinglePixelPackedSampleModel,MultiPixelPackedSampleModel) were replayed through the new code with their Java-produced bank data and read back unchanged. All of them use zero buffer offsets, an identity bank mapping or in-range band offsets, which is whytests/raster/test_serde.pydid not catch any of these defects.Checked under numpy 1.26 and numpy 2.5.
Did this PR include necessary documentation updates?