From a83aac328ce76eed177508dd79d4f96c4fed6e24 Mon Sep 17 00:00:00 2001 From: s-sasaki-earthsea-wizard Date: Sun, 10 May 2026 23:40:29 +0900 Subject: [PATCH 1/2] Restore = nullptr defaults to CUDA Rdr2Geo.topo() optional rasters The CUDA pybind binding for isce3::cuda::geometry::Topo::topo() declared all 11 output raster kwargs as required, while the underlying C++ class accepts nullptr per its documented contract and the CPU sibling binding marks the same kwargs `= nullptr`. Calling the CUDA overload from a context that omits a subset of layers (e.g. COMPASS s1_rdr2geo.run) raised TypeError on argument matching. Restore the symmetric `= nullptr` defaults to match the CPU binding and the C++ class signature. Also rename `local_Psi_raster` to `local_psi_raster` to align with the CPU binding's lowercase form. The defaults were originally added symmetrically to both CPU and CUDA bindings in the "Optional topo layers" change. They were silently dropped on the CUDA side when the binding block was rewritten in a different formatting style for the ground-to-sat East/North components extension; the CPU sibling was edited in place and kept its defaults intact. No test exercises the optional path on either side, so CI stayed green. --- .../pybind_isce3/cuda/geometry/rdr2geo.cpp | 22 +++++++++---------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/python/extensions/pybind_isce3/cuda/geometry/rdr2geo.cpp b/python/extensions/pybind_isce3/cuda/geometry/rdr2geo.cpp index 671e1de8d..695fa5e48 100644 --- a/python/extensions/pybind_isce3/cuda/geometry/rdr2geo.cpp +++ b/python/extensions/pybind_isce3/cuda/geometry/rdr2geo.cpp @@ -65,17 +65,17 @@ void addbinding(py::class_ & pyRdr2Geo) isce3::io::Raster*> (&Topo::topo), py::arg("dem_raster"), - py::arg("x_raster"), - py::arg("y_raster"), - py::arg("height_raster"), - py::arg("incidence_angle_raster"), - py::arg("heading_angle_raster"), - py::arg("local_incidence_angle_raster"), - py::arg("local_Psi_raster"), - py::arg("simulated_amplitude_raster"), - py::arg("layover_shadow_raster"), - py::arg("ground_to_sat_east_raster"), - py::arg("ground_to_sat_north_raster"), + py::arg("x_raster") = nullptr, + py::arg("y_raster") = nullptr, + py::arg("height_raster") = nullptr, + py::arg("incidence_angle_raster") = nullptr, + py::arg("heading_angle_raster") = nullptr, + py::arg("local_incidence_angle_raster") = nullptr, + py::arg("local_psi_raster") = nullptr, + py::arg("simulated_amplitude_raster") = nullptr, + py::arg("layover_shadow_raster") = nullptr, + py::arg("ground_to_sat_east_raster") = nullptr, + py::arg("ground_to_sat_north_raster") = nullptr, R"( Run topo and output to user created topo rasters From d9e2d6703014dba60ac8c15d0c739de6032fff59 Mon Sep 17 00:00:00 2001 From: s-sasaki-earthsea-wizard Date: Sun, 10 May 2026 23:40:47 +0900 Subject: [PATCH 2/2] Add CUDA Rdr2Geo.topo() optional-rasters regression test Add a CUDA-side test that calls topo() with the call shape used by COMPASS s1_rdr2geo.run: kwargs for a subset of output layers as Rasters, others passed as None or omitted entirely. This exercises both the binding-layer kwarg defaults and pybind11's None-to-nullptr conversion for pointer types. Without this test, binding-layer regressions on the optional path go undetected because test_run_raster_layers always passes every kwarg explicitly. Also gate the whole module with `pytestmark = pytest.mark.skipif` on `isce3.cuda` availability so CPU-only builds report a clean skip with reason rather than a fixture collection error. --- .../pybind/cuda/geometry/rdr2geo.py | 51 +++++++++++++++++++ 1 file changed, 51 insertions(+) diff --git a/tests/python/extensions/pybind/cuda/geometry/rdr2geo.py b/tests/python/extensions/pybind/cuda/geometry/rdr2geo.py index 7932acc6c..424fddb36 100644 --- a/tests/python/extensions/pybind/cuda/geometry/rdr2geo.py +++ b/tests/python/extensions/pybind/cuda/geometry/rdr2geo.py @@ -11,6 +11,16 @@ import isce3 from nisar.products.readers import SLC +# CUDA-gated: skip the whole module on CPU-only isce3 builds. The tests below +# all depend on isce3.cuda.geometry.Rdr2Geo, which only exists when isce3 was +# configured with CUDA. Without this gate, collection on a CPU-only build +# would surface as fixture errors rather than a clear skip with a reason. +pytestmark = pytest.mark.skipif( + not hasattr(isce3, "cuda"), + reason="isce3 built without CUDA support; skipping isce3.cuda.geometry tests", +) + + @pytest.fixture(scope="module") def unit_test_params(): params = types.SimpleNamespace() @@ -129,6 +139,47 @@ def test_run_raster_layers(unit_test_params): ) +def test_run_optional_layers(unit_test_params): + ''' + Exercise the optional-raster path of topo() in the same call shape used + by COMPASS s1_rdr2geo.run: kwargs for a subset of output layers, with + explicit None for some and others omitted entirely. The C++ class + isce3::cuda::geometry::Topo::topo() documents an all-nullptr default + contract for the 11 output rasters, mirrored on the CPU sibling + binding. Without this test, the binding-layer regression that drops + the `= nullptr` defaults is invisible to CI because the existing + test_run_raster_layers passes every kwarg explicitly. + ''' + radargrid = unit_test_params.radargrid + length, width = radargrid.shape + + # Subset matching COMPASS's typical call: x/y/height + layoverShadow, + # plus a few explicit Nones (exercising pybind11's None-to-nullptr + # conversion) and the remaining four output rasters omitted entirely + # (exercising the kwarg defaults). + fnames_dtypes = [ + ("opt_x", gdal.GDT_Float64), + ("opt_y", gdal.GDT_Float64), + ("opt_z", gdal.GDT_Float64), + ("opt_layoverShadow", gdal.GDT_Byte), + ] + x_raster, y_raster, height_raster, layover_shadow_raster = [ + isce3.io.Raster(f"{fname}.rdr", width, length, 1, dtype, "ENVI") + for fname, dtype in fnames_dtypes + ] + + unit_test_params.rdr2geo_obj.topo( + unit_test_params.dem_raster, + x_raster=x_raster, + y_raster=y_raster, + height_raster=height_raster, + local_incidence_angle_raster=None, + layover_shadow_raster=layover_shadow_raster, + ground_to_sat_east_raster=None, + ground_to_sat_north_raster=None, + ) + + def test_validate(unit_test_params): """ validate generated results