Skip to content

Comments

Warn about INIT_DEST in warp operation instead of failing#12189

Closed
sgillies wants to merge 2 commits intomasterfrom
sgillies-init-dest-warn-noop
Closed

Warn about INIT_DEST in warp operation instead of failing#12189
sgillies wants to merge 2 commits intomasterfrom
sgillies-init-dest-warn-noop

Conversation

@sgillies
Copy link
Contributor

@sgillies sgillies commented Apr 21, 2025

#11978 changed a no-op condition to a hard failure. This breaks every recent version of Rasterio, which sets INIT_DEST=NO_DATA by default: https://github.com/rasterio/rasterio/blob/0fe62a7106034021e0375746b2581ad595f44f86/rasterio/_warp.pyx#L584-L585.

Instead, how about restoring the no-op, but with a warning?

@sgillies sgillies added the regression Bug that did not exist in a prior release label Apr 21, 2025
@jratike80
Copy link
Collaborator

If I understand right, the gdalwarp binary has always required some input for the destination nodata

gdalwarp  -dstnodata test.tif nodata.tif --debug on 

ERROR 1: Missing dst_dataset_name

Out of curiosity, what Rasterio is doing with such input? Does it copy the source nodata value, or set nodata into 0, or what?

@sgillies
Copy link
Contributor Author

@jratike80 Rasterio uses GDAL's C++ warper, not gdalwarp or its core C function.

@jratike80
Copy link
Collaborator

Rasterio uses GDAL's C++ warper, not gdalwarp or its core C function.

I guessed that, but I was thinking that Rasrerio it is doing something similar than gdalwarp and there is something in common in the logic.

This is fun:

gdal_create -outsize 100 100 -ot byte -burn 100 -a_nodata 255 test.tif
gdalwarp -dstnodata foo test.tif nodatatest.tif -to SRC_METHOD=NO_GEOTRANSFORM
gdalinfo nodatatest.tif
...
Band 1 Block=100x81 Type=Byte, ColorInterp=Gray
  NoData Value=0

This is with GDAL 3.11.0dev-29aee5da70, released 2025/04/10
Generally speaking I think it would be good to validate INIT_DEST/destination nodata etc. but I understand that because of existing Rasterio usage it might break too much.

@coveralls
Copy link
Collaborator

coveralls commented Apr 21, 2025

Coverage Status

coverage: 70.719% (+0.01%) from 70.707%
when pulling 73a75a8 on sgillies-init-dest-warn-noop
into bfb3825 on master.

@sgillies sgillies requested review from dbaston and rouault and removed request for rouault April 21, 2025 23:17
@rouault
Copy link
Member

rouault commented Apr 22, 2025

Sean isn't possible to modify instead Rasterio to not set INIT_DEST to NO_DATA when there is none but instead likely to 0 ?

@dbaston
Copy link
Member

dbaston commented Apr 22, 2025

#11978 changed a no-op condition to a hard failure

It's not a no-op -- it causes an initialization to zero, though nothing in the code suggests to me that this behavior is intentional. Try running the below snippet both with and without INIT_DEST.

src = gdal.GetDriverByName("MEM").Create("", 1, 1)
src.SetGeoTransform((0, 1, 0, 1, 0, -1))
src.GetRasterBand(1).Fill(7)

dst = gdal.GetDriverByName("MEM").Create("", 2, 2)
dst.SetGeoTransform((0, 1, 0, 2, 0, -1))
dst.GetRasterBand(1).Fill(1)

gdal.Warp(dst, src, warpOptions={"INIT_DEST":"NO_DATA"})

print(dst.ReadAsArray())

If the desired behavior in rasterio is "initialize to the NoData unless there isn't one, in which case initialize to zero", why not code that explicitly?

@sgillies
Copy link
Contributor Author

A new Rasterio release to fix its usage isn't in the cards right now. That's why I'm asking if we can have a warning instead of a warp-stopping failure.

Now it could be that Rasterio user code is less susceptible that Rasterio's tests, and that I could just update Rasterio's tests to not blindly set INIT_DEST like they do. I'll look into that.

@dbaston
Copy link
Member

dbaston commented Apr 22, 2025

I don't have any problem making it a warning for the current release, I'd just prefer not to enshrine the current behavior. Would you object to making it a failure in 3.12?

dbaston added a commit to dbaston/gdal that referenced this pull request Apr 22, 2025
rouault pushed a commit to dbaston/gdal that referenced this pull request Apr 27, 2025
@rouault
Copy link
Member

rouault commented Apr 27, 2025

superseded per #12198

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

regression Bug that did not exist in a prior release

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants