isce-framework / isce-framework/isce3

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

Open Beginner friendly
#353 0 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

Dominant language
Python
Stars
239
Forks
90
Avg merge
13d 1h
Merged PRs (30d)
5

Description

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.

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start by comparing the topo() bindings in python/extensions/pybind_isce3/cuda/geometry/rdr2geo.cpp and python/extensions/pybind_isce3/geometry/rdr2geo.cpp. Run the supplied minimal reproduction against CPU and CUDA builds; done means the CUDA binding accepts omitted raster keywords consistently, with the local_psi_raster naming discrepancy addressed as maintainers decide.

Written by the indexing model from the issue text.

Assessment

Tech stack
cpp, python
Domain
api, backend
Issue type
Bug
Difficulty
2/5
Estimated time
1-3 hours
Activity status
Quiet
Clarity
Clearly specified
Newbie friendliness
76/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.