Skip to content

cuda.geometry.Rdr2Geo.topo() pybind11 binding is missing default arguments that geometry.Rdr2Geo.topo() (CPU) has #353

Description

@JIN-Z-pop

Summary

The CUDA-accelerated isce3.cuda.geometry.Rdr2Geo.topo() Python binding requires all 11
output-raster keyword arguments to be supplied, while the CPU isce3.geometry.Rdr2Geo.topo()
binding gives all 11 of them a None/nullptr default so callers may omit the ones they
don't need. This asymmetry is not documented and is not obviously intentional — it appears to
be an oversight that has never been synced between the two binding files. It breaks any caller
that follows the pattern the CPU binding's own docstring recommends (only pass the rasters you
actually want), the moment that caller is run against the CUDA build instead of the CPU build.

A downstream project (opera-adt/COMPASS, s1_rdr2geo.py) calls Rdr2Geo.topo() with exactly
7 of the 11 keyword arguments (omitting incidence_angle_raster, heading_angle_raster,
local_psi_raster/local_Psi_raster, simulated_amplitude_raster), which works fine against
isce3.geometry.Rdr2Geo but raises TypeError against isce3.cuda.geometry.Rdr2Geo with the
identical call.

Environment

  • isce3 versions observed: 0.25.8 (pixi/conda-forge, cuda129 build) and 0.25.12 (miniforge3,
    cpu-only build). Both show the same asymmetry.
  • Confirmed still present on develop HEAD (default branch) as of 2026-08-13, and on the
    latest tagged release v0.25.16.
  • Both binding source files
    (python/extensions/pybind_isce3/geometry/rdr2geo.cpp and
    python/extensions/pybind_isce3/cuda/geometry/rdr2geo.cpp)
    were inspected directly via the GitHub API (no local checkout needed).
    cuda/geometry/rdr2geo.cpp's last commit touching it is dated 2023-07-24 — it has not
    been modified since, across all isce3 releases from before 0.25.8 through 0.25.16.
  • Downstream caller: opera-adt/COMPASS, compass/s1_rdr2geo.py, function used inside
    compass/s1_static_layers.py (static-layer product generation workflow).

Steps to reproduce

Minimal reproduction, no real SAR data required — a 2x2-pixel dummy DEM/output raster set and
a synthetic orbit/radar-grid are enough to hit the argument-binding error (it fails before any
real geometric computation happens):

import isce3
from osgeo import gdal, osr
import tempfile, os

ellipsoid = isce3.core.Ellipsoid(6378137.0, 0.0066943799901413165)
ref_epoch = isce3.core.DateTime(2026, 1, 1, 0, 0, 0.0)
svs = [isce3.core.StateVector(isce3.core.DateTime(2026, 1, 1, 0, 0, float(i)),
                               [7000000.0 + i * 100.0, 100000.0, 200000.0],
                               [10.0, 7500.0, 10.0]) for i in range(5)]
orbit = isce3.core.Orbit(svs, ref_epoch)
rdr_grid = isce3.product.RadarGridParameters(
    1.0, 0.05546576, 1000.0, 800000.0, 5.0,
    isce3.core.LookSide.Right, 2, 2, ref_epoch)
grid_doppler = isce3.core.LUT2d()

scratch = tempfile.mkdtemp()
dem_path = os.path.join(scratch, "dem.tif")
ds = gdal.GetDriverByName("GTiff").Create(dem_path, 2, 2, 1, gdal.GDT_Float32)
ds.GetRasterBand(1).Fill(10.0)
ds.SetGeoTransform([130.0, 0.001, 0, 33.0, 0, -0.001])
srs = osr.SpatialReference(); srs.ImportFromEPSG(4326)
ds.SetProjection(srs.ExportToWkt()); ds = None
dem_raster = isce3.io.Raster(dem_path)

def out(name, dtype):
    p = os.path.join(scratch, f"{name}.tif")
    return isce3.io.Raster(p, 2, 2, 1, dtype, "GTiff")

kwargs = dict(
    x_raster=out("x", gdal.GDT_Float64),
    y_raster=out("y", gdal.GDT_Float64),
    height_raster=out("height", gdal.GDT_Float64),
    local_incidence_angle_raster=out("lia", gdal.GDT_Float32),
    layover_shadow_raster=out("ls", gdal.GDT_Byte),
    ground_to_sat_east_raster=out("east", gdal.GDT_Float32),
    ground_to_sat_north_raster=out("north", gdal.GDT_Float32),
)

# Works:
isce3.geometry.Rdr2Geo(rdr_grid, orbit, ellipsoid, grid_doppler).topo(dem_raster, **kwargs)

# Raises TypeError with the identical kwargs:
isce3.cuda.geometry.Rdr2Geo(rdr_grid, orbit, ellipsoid, grid_doppler).topo(dem_raster, **kwargs)

Expected behavior

isce3.cuda.geometry.Rdr2Geo.topo() accepts the same partial-kwargs call as
isce3.geometry.Rdr2Geo.topo(), since both wrap the same conceptual Topo::topo() C++ overload
and are documented with (almost) identical docstrings.

Actual behavior

TypeError: topo(): incompatible function arguments. The following argument types are supported:
    1. (self: isce3.ext.isce3.cuda.geometry.Rdr2Geo, dem_raster: isce3.ext.isce3.io.Raster, outdir: str) -> None
    2. (self: isce3.ext.isce3.cuda.geometry.Rdr2Geo, dem_raster: isce3.ext.isce3.io.Raster, x_raster: isce3.ext.isce3.io.Raster, y_raster: isce3.ext.isce3.io.Raster, height_raster: isce3.ext.isce3.io.Raster, incidence_angle_raster: isce3.ext.isce3.io.Raster, heading_angle_raster: isce3.ext.isce3.io.Raster, local_incidence_angle_raster: isce3.ext.isce3.io.Raster, local_Psi_raster: isce3.ext.isce3.io.Raster, simulated_amplitude_raster: isce3.ext.isce3.io.Raster, layover_shadow_raster: isce3.ext.isce3.io.Raster, ground_to_sat_east_raster: isce3.ext.isce3.io.Raster, ground_to_sat_north_raster: isce3.ext.isce3.io.Raster) -> None

Invoked with: <Rdr2Geo object>, <Raster object>; kwargs: x_raster=..., y_raster=..., height_raster=..., local_incidence_angle_raster=..., layover_shadow_raster=..., ground_to_sat_east_raster=..., ground_to_sat_north_raster=...

Root cause

Source-level comparison of the two pybind11 binding files confirms the asymmetry directly
(not inferred from behavior alone):

CPUpython/extensions/pybind_isce3/geometry/rdr2geo.cpp:

py::arg("dem_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,

CUDApython/extensions/pybind_isce3/cuda/geometry/rdr2geo.cpp:

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"),

None of the CUDA-side arguments have a default value, so pybind11's overload resolution
requires every one of them whenever a caller uses keyword arguments and skips any of the
optional ones.

A secondary, independent finding from the same comparison: the CPU binding names one parameter
local_psi_raster (lowercase p) while the CUDA binding names the equivalent parameter
local_Psi_raster (uppercase P). This is minor by itself, but it corroborates that the two
binding files were written/maintained independently and have drifted out of sync.

Real-world impact

opera-adt/COMPASS's compass/s1_rdr2geo.py selects isce3.cuda.geometry.Rdr2Geo whenever
isce3.core.gpu_check.use_gpu(cfg.gpu_enabled, cfg.gpu_id) is true, then calls .topo() with
7 of the 11 raster kwargs (intentionally omitting the 4 it doesn't need, following the pattern
documented for the CPU binding). This raises TypeError unconditionally on any GPU-enabled run
that reaches the static-layers stage, regardless of DEM/orbit/burst data — i.e. it is a total,
data-independent blocker for GPU-accelerated static-layer product generation.

Workaround (in use downstream)

Force the CPU class for the static-layers stage specifically (leaving the rest of the pipeline
on GPU), by setting the existing gpu_enabled config flag to False before constructing
Rdr2Geo for that stage only:

rdr2geo_cfg.groups.worker.gpu_enabled = False
assert not isce3.core.gpu_check.use_gpu(rdr2geo_cfg.gpu_enabled, rdr2geo_cfg.gpu_id)

This was verified end-to-end against a real Sentinel-1 burst (existing GPU-geocoded SLC as
input): s1_static_layers workflow completes in ~1 minute wall-clock on CPU, producing a valid
static-layers HDF5 product, with no other pipeline stage affected.

Suggested fix

Add the same 11 = nullptr default values to
python/extensions/pybind_isce3/cuda/geometry/rdr2geo.cpp's topo() binding that
python/extensions/pybind_isce3/geometry/rdr2geo.cpp already has, and rename
local_Psi_raster to local_psi_raster for consistency (note: the rename is a breaking change
for any caller currently passing that one argument by keyword on the CUDA class — may warrant a
deprecation cycle or an aliased kwarg).

Open question for maintainers

Is the missing-defaults state intentional (e.g. a deliberate constraint on the CUDA kernel
launch requiring every buffer to be pre-allocated), or simply unsynced with the CPU binding
since whichever of the two got default arguments added first? If intentional, updating the
docstring/type stubs to make the CPU/CUDA classes' signatures explicitly asymmetric (rather
than presenting near-identical docstrings) would help callers avoid this trap.

Activity

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

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions