Python bindings: make GetStatistics() and ComputeStatistics() return …

…None in case of error (OSGeo#10462) This was the originally intended behavior, but the IF_ERROR_RETURN_NONE typemap that controls this was actually broken. It used to work before 2006, but it was removed per commit 1c6e19f and later half restored in 86d4f1d , missing the important 'ret' typemap that was initially present.
rouault · Jul 22, 2024 · b1b67c8 · b1b67c8
1 parent b4fce48
commit b1b67c8
Show file tree

Hide file tree

Showing 12 changed files with 30 additions and 36 deletions.
diff --git a/MIGRATION_GUIDE.TXT b/MIGRATION_GUIDE.TXT
@@ -15,6 +15,9 @@ MIGRATION GUIDE FROM GDAL 3.9 to GDAL 3.10
   corresponding optional (but recommended to be implemented to reliably detect
   reading errors) callbacks "error" and "clear_err".
 
+- Python bindings: Band.GetStatistics() and Band.ComputeStatistics() now
+  return a None value in case of error (when exceptions are not enabled)
+
 MIGRATION GUIDE FROM GDAL 3.8 to GDAL 3.9
 -----------------------------------------
 

diff --git a/autotest/gcore/gdal_stats.py b/autotest/gcore/gdal_stats.py
@@ -99,7 +99,7 @@ def test_stats_dont_force():
         gdal.Unlink("data/byte.tif.aux.xml")
     ds = gdal.Open("data/byte.tif")
     stats = ds.GetRasterBand(1).GetStatistics(0, 0)
-    assert stats == [0, 0, 0, -1], "did not get expected stats"
+    assert stats is None
 
 
 ###############################################################################
@@ -762,7 +762,7 @@ def test_stats_approx_stats_flag(dt=gdal.GDT_Byte, struct_frmt="B"):
     approx_ok = 0
     force = 0
     stats = ds.GetRasterBand(1).GetStatistics(approx_ok, force)
-    assert stats == [0.0, 0.0, 0.0, -1.0], "did not get expected stats"
+    assert stats is None
 
     approx_ok = 0
     force = 1
@@ -824,15 +824,15 @@ def test_stats_clear():
     filename = "/vsimem/out.tif"
     gdal.Translate(filename, "data/byte.tif")
     ds = gdal.Open(filename)
-    assert ds.GetRasterBand(1).GetStatistics(False, False) == [0, 0, 0, -1]
-    assert ds.GetRasterBand(1).ComputeStatistics(False) != [0, 0, 0, -1]
+    assert ds.GetRasterBand(1).GetStatistics(False, False) is None
+    assert ds.GetRasterBand(1).ComputeStatistics(False) is not None
 
     ds = gdal.Open(filename)
-    assert ds.GetRasterBand(1).GetStatistics(False, False) != [0, 0, 0, -1]
+    assert ds.GetRasterBand(1).GetStatistics(False, False) is not None
     ds.ClearStatistics()
 
     ds = gdal.Open(filename)
-    assert ds.GetRasterBand(1).GetStatistics(False, False) == [0, 0, 0, -1]
+    assert ds.GetRasterBand(1).GetStatistics(False, False) is None
 
     gdal.GetDriverByName("GTiff").Delete(filename)
 

diff --git a/autotest/gcore/pam.py b/autotest/gcore/pam.py
@@ -382,7 +382,7 @@ def test_pam_11():
     # Check that we actually have no saved statistics
     ds = gdal.Open("tmpdirreadonly/byte.tif")
     stats = ds.GetRasterBand(1).GetStatistics(False, False)
-    assert stats[3] == -1, "did not expected to have stats at that point"
+    assert stats is None
     ds = None
 
     # This must be run as an external process so we can override GDAL_PAM_PROXY_DIR

diff --git a/autotest/gcore/vrt_read.py b/autotest/gcore/vrt_read.py
@@ -2341,7 +2341,7 @@ def test_vrt_read_compute_statistics_mosaic_optimization_src_with_nodata_all():
 
     with gdal.quiet_errors():
         vrt_stats = vrt_ds.GetRasterBand(1).ComputeStatistics(False)
-        assert vrt_stats == [0, 0, 0, 0]
+        assert vrt_stats is None
         assert vrt_ds.GetRasterBand(1).GetMetadataItem("STATISTICS_MINIMUM") is None
 
 

diff --git a/autotest/gdrivers/ecw.py b/autotest/gdrivers/ecw.py
@@ -1479,7 +1479,7 @@ def test_ecw_41(tmp_path):
     # Check that no statistics is already included in the file
     assert ds.GetRasterBand(1).GetMinimum() is None
     assert ds.GetRasterBand(1).GetMaximum() is None
-    assert ds.GetRasterBand(1).GetStatistics(1, 0) == [0.0, 0.0, 0.0, -1.0]
+    assert ds.GetRasterBand(1).GetStatistics(1, 0) is None
     assert ds.GetRasterBand(1).GetDefaultHistogram(force=0) is None
 
     # Now compute the stats

diff --git a/autotest/gdrivers/ehdr.py b/autotest/gdrivers/ehdr.py
@@ -379,20 +379,20 @@ def test_ehdr_approx_stats_flag():
     approx_ok = 1
     force = 1
     stats = ds.GetRasterBand(1).GetStatistics(approx_ok, force)
-    assert stats == [0.0, 0.0, 0.0, 0.0], "did not get expected stats"
+    assert stats == [0.0, 0.0, 0.0, 0.0]
     md = ds.GetRasterBand(1).GetMetadata()
     assert "STATISTICS_APPROXIMATE" in md, "did not get expected metadata"
 
     approx_ok = 0
     force = 0
     stats = ds.GetRasterBand(1).GetStatistics(approx_ok, force)
-    assert stats == [0.0, 0.0, 0.0, -1.0], "did not get expected stats"
+    assert stats is None
 
     ds = gdal.Open(tmpfile, gdal.GA_Update)
     approx_ok = 0
     force = 0
     stats = ds.GetRasterBand(1).GetStatistics(approx_ok, force)
-    assert stats == [0.0, 0.0, 0.0, -1.0], "did not get expected stats"
+    assert stats is None
 
     approx_ok = 0
     force = 1

diff --git a/autotest/gdrivers/netcdf_multidim.py b/autotest/gdrivers/netcdf_multidim.py
@@ -3979,12 +3979,7 @@ def test():
 
         view = ar.GetView("[0:10,...]")
         classic_ds = view.AsClassicDataset(1, 0)
-        assert classic_ds.GetRasterBand(1).GetStatistics(False, False) == [
-            0.0,
-            0.0,
-            0.0,
-            -1.0,
-        ]
+        assert classic_ds.GetRasterBand(1).GetStatistics(False, False) is None
         classic_ds.GetRasterBand(1).ComputeStatistics(False)
 
         view = ar.GetView("[10:20,...]")
@@ -4035,12 +4030,7 @@ def reopen():
         )
 
         classic_ds = ar.AsClassicDataset(1, 0)
-        assert classic_ds.GetRasterBand(1).GetStatistics(False, False) == [
-            0.0,
-            0.0,
-            0.0,
-            -1.0,
-        ]
+        assert classic_ds.GetRasterBand(1).GetStatistics(False, False) is None
 
         rg_subset = rg.SubsetDimensionFromSelection("/x=440750")
 

diff --git a/autotest/gdrivers/nitf.py b/autotest/gdrivers/nitf.py
@@ -1488,8 +1488,7 @@ def test_nitf_36():
         ds.GetRasterBand(1).GetMinimum() is None
     ), "Did not expect to have minimum value at that point."
 
-    (_, _, mean, stddev) = ds.GetRasterBand(1).GetStatistics(False, False)
-    assert stddev < 0, "Did not expect to have statistics at that point."
+    assert ds.GetRasterBand(1).GetStatistics(False, False) is None
 
     (exp_mean, exp_stddev) = (65.4208, 47.254550335)
     (_, _, mean, stddev) = ds.GetRasterBand(1).GetStatistics(False, True)

diff --git a/autotest/gdrivers/vrtprocesseddataset.py b/autotest/gdrivers/vrtprocesseddataset.py
@@ -1181,7 +1181,7 @@ def test_vrtprocesseddataset_serialize(tmp_vsimem):
     with gdaltest.tempfile(vrt_filename, content):
         ds = gdal.Open(vrt_filename)
         np.testing.assert_equal(ds.GetRasterBand(1).ReadAsArray(), np.array([[11, 12]]))
-        assert ds.GetRasterBand(1).GetStatistics(False, False) == [0.0, 0.0, 0.0, -1.0]
+        assert ds.GetRasterBand(1).GetStatistics(False, False) is None
         ds.GetRasterBand(1).ComputeStatistics(False)
         ds.Close()
 

diff --git a/swig/include/python/typemaps_python.i b/swig/include/python/typemaps_python.i
@@ -225,6 +225,15 @@ TYPEMAP_ARGOUT_ARGOUT_ARRAY_IS_VALID(6)
   /* %typemap(out) IF_ERROR_RETURN_NONE */
 }
 
+%typemap(ret) IF_ERROR_RETURN_NONE
+{
+  /* %typemap(ret) IF_ERROR_RETURN_NONE */
+  if ($1 != CE_None ) {
+    Py_XDECREF( $result );
+    $result = Py_None;
+    Py_INCREF($result);
+  }
+}
 
 %import "ogr_error_map.i"
 

diff --git a/swig/python/gdal-utils/osgeo_utils/samples/gdalinfo.py b/swig/python/gdal-utils/osgeo_utils/samples/gdalinfo.py
@@ -402,9 +402,8 @@ def main(argv=None):
             print(line)
 
         stats = hBand.GetStatistics(bApproxStats, bStats)
-        # Dirty hack to recognize if stats are valid. If invalid, the returned
-        # stddev is negative
-        if stats[3] >= 0.0:
+        # Before GDAL 3.10, a negative value for stddev indicated an error
+        if stats is not None and stats[3] >= 0.0:
             print(
                 "  Minimum=%.3f, Maximum=%.3f, Mean=%.3f, StdDev=%.3f"
                 % (stats[0], stats[1], stats[2], stats[3])

diff --git a/swig/python/modify_cpp_files.cmake b/swig/python/modify_cpp_files.cmake
@@ -33,12 +33,6 @@ string(REPLACE "obj = PyUnicode_AsUTF8String(obj);"
                "obj = PyUnicode_AsUTF8String(obj); if (!obj) return SWIG_TypeError;"
        _CONTENTS "${_CONTENTS}")
 
-if("${FILE}" MATCHES "gdal_wrap.cpp")
-    string(REGEX REPLACE "result = \\(CPLErr\\)([^;]+)(\\;)"
-                         [[CPL_IGNORE_RET_VAL(result = (CPLErr)\1)\2]]
-           _CONTENTS "${_CONTENTS}")
-endif()
-
 string(REPLACE "PyObject *resultobj = 0;"
                "PyObject *resultobj = 0; int bLocalUseExceptionsCode = GetUseExceptions();"
        _CONTENTS "${_CONTENTS}")