[GH-3112] RS_AsRaster fills uncovered pixels with noDataValue#3114
[GH-3112] RS_AsRaster fills uncovered pixels with noDataValue#3114james-willis wants to merge 3 commits into
Conversation
…against rasterio" This reverts commit 7085eab.
| } | ||
| int width = writableRaster.getWidth(); | ||
| double[] row = new double[width]; | ||
| Arrays.fill(row, backgroundValue); |
There was a problem hiding this comment.
Would it be worth normalizing backgroundValue to pixelType before writing it here? I may be missing validation elsewhere, but the integer sample conversion seems able to produce a pixel value that differs from the nodata metadata.
For example:
WITH r AS (
SELECT RS_AsRaster(
ST_GeomFromWKT('POLYGON ((0 3, 1 3, 1 2, 0 2, 0 3))'),
RS_MakeEmptyRaster(1, 'B', 3, 3, 0.0, 3.0, 1.0, -1.0, 0.0, 0.0, 0),
'B', false, 1.0, -1.0, false
) AS rast
)
SELECT RS_BandAsArray(rast, 1), RS_BandNoDataValue(rast, 1)
FROM r;On this branch I get background pixels of 255, while RS_BandNoDataValue returns -1.0. Changing -1.0 to 300.0 gives background pixels of 44 with nodata metadata 300.0.
That would leave the background counted as data for those inputs. PostGIS looks like it clamps/truncates nodata to the band pixel type. Could we either convert once and use the converted value for both the fill and metadata, or reject values that the pixel type cannot represent?
| * `allTouched` (Since: `v1.7.1`): Decides the pixel selection criteria. If set to `true`, the function selects all pixels touched by the geometry, else, selects only pixels whose centroids intersect the geometry. Defaults to `false`. | ||
| * `value`: The value to be used for assigning pixels covered by the geometry. Defaults to using `1.0` if not provided. | ||
| * `noDataValue`: Used for assigning the no data value of the resultant raster. Defaults to `null` if not provided. | ||
| * `noDataValue`: Used for assigning the no data value of the resultant raster; pixels not covered by the geometry are set to this value. Defaults to `null` if not provided, in which case uncovered pixels are `0` and the raster has no no data value. |
There was a problem hiding this comment.
One separate compatibility question: do we want omission to continue meaning null here? I may be reading the PostGIS behavior too literally, but its scalar overloads declare nodataval DEFAULT 0, and omission is observably different from passing NULL.
This PostGIS query shows the distinction:
WITH g AS (
SELECT ST_GeomFromText(
'POLYGON ((0 0, 3 0, 3 3, 0 3, 0 0),
(1 1, 2 1, 2 2, 1 2, 1 1))'
) AS geom
),
r AS (
SELECT
ST_AsRaster(geom, 1.0, -1.0, '8BUI') AS omitted,
ST_AsRaster(
geom, 1.0, -1.0, '8BUI', 1.0, NULL::double precision
) AS explicit_null
FROM g
)
SELECT
ST_BandNoDataValue(omitted, 1),
ST_Value(omitted, 1, 2, 2, false),
ST_BandNoDataValue(explicit_null, 1),
ST_Value(explicit_null, 1, 2, 2, false)
FROM r;I get 0 | 0 | NULL | 0: both backgrounds contain zero, but only the omitted case marks zero as nodata.
The comparable Sedona check is:
WITH inputs AS (
SELECT
ST_GeomFromWKT(
'POLYGON ((0 0, 3 0, 3 3, 0 3, 0 0),
(1 1, 2 1, 2 2, 1 2, 1 1))'
) AS geom,
RS_MakeEmptyRaster(
1, 'B', 3, 3, 0.0, 3.0, 1.0, -1.0, 0.0, 0.0, 0
) AS ref
),
r AS (
SELECT
RS_AsRaster(geom, ref, 'B') AS omitted,
RS_AsRaster(geom, ref, 'B', false, 1.0, 0.0) AS explicit_zero
FROM inputs
)
SELECT
RS_BandNoDataValue(omitted, 1),
RS_BandNoDataValue(explicit_zero, 1)
FROM r;That gives NULL | 0. I can see a compatibility reason to preserve the existing Sedona default, so this may be better handled separately. If matching PostGIS defaults is the goal, though, the shorter overloads would need to pass 0, while explicit NULL could keep the current no-nodata behavior.
Did you read the Contributor Guide?
Is this PR related to a ticket?
[GH-XXX] my subject. Closes RS_AsRaster: pixels not covered by the geometry get value 0 instead of noDataValue #3112What changes were proposed in this PR?
RS_AsRasterpreviously only attachednoDataValueas band metadata; pixels not covered by the geometry kept the allocation default of0, a valid pixel value indistinguishable from data. NowRasterizationfills the freshly-allocated raster with thenoDataValuebefore burning the geometry, so uncovered pixels (including interior holes) read back as the declared nodata — matching PostGISST_AsRaster(nodatavalfills untouched cells) and thegdal_rasterize -init <nodata> -a_nodata <nodata>idiom. Passing nonoDataValuekeeps the current all-zero background.The fill happens before burning (not after) because a burn
valueof0would otherwise be indistinguishable from background.RS_SetValuesinternally rasterized its geometry mask with the band's nodata attached; since it reads that raster purely as a mask where0means "outside the geometry", it now uses the overload without a nodata, preserving its behavior exactly.Existing test expectations that asserted
0backgrounds alongside a non-zeronoDataValueare updated to expect the nodata — those expectations were pinning the bug from #3112.How was this patch tested?
testAsRasterBackgroundIsNoDataValue: a polygon with an interior hole where the hole and the outside must read back as the nodata; avalue = 0burn that must stay distinguishable from the nodata background; the geometry-extent output path; and the null-nodata default keeping0. The test was written first and fails on master.commonmodule suite passes.PixelFunctionEditorsTest(RS_SetValues),RasterBandAccessorsTest(zonal stats, which passesnullnodata and relies on0backgrounds), andRasterBandEditorsTest(RS_Clip, which never passes a nodata) pass unchanged — the consumers that depend on0-backed masks are unaffected.Did this PR include necessary documentation updates?