Regression: CUDA Rdr2Geo.topo() pybind binding lost optional-raster defaults

[s-sasaki-earthsea-wizard]

Summary

The Python bindings isce3.geometry.Rdr2Geo.topo (CPU) and
isce3.cuda.geometry.Rdr2Geo.topo (CUDA) are pybind11 wrappers around the
same C++ method
isce3::cuda::geometry::Topo::topo(),
which accepts isce3::io::Raster* (nullable) for every output-layer argument.
Both bindings ship with the same docstring describing the same
optional-output-layers contract.

On current develop, however, the CUDA binding has zero = None keyword
defaults and rejects the call shape that the C++ class accepts and the CPU
sibling accepts. The minimal failing call is:

import isce3.cuda.geometry as g
rdr2geo = g.Rdr2Geo(radar_grid, orbit, ellipsoid, doppler)
rdr2geo.topo(dem_raster, x_raster=x, y_raster=y, height_raster=z)
# TypeError: topo(): incompatible function arguments

The CPU sibling accepts the same call and produces the requested layers.

Evidence (purely within isce3)

A standalone script that imports only isce3 and uses isce3's own test
fixtures (envisat.h5, srtm_cropped.tif from tests/data/) reproduces
the divergence in two layers. The full script and captured outputs are
linked in the "Reproducing" section below.

Layer 1 — pybind11 docstring signature inspection (counts = None
defaults in the multi-raster topo() overload for each sibling, on
develop HEAD 9552d682):

-- CPU  isce3.geometry.Rdr2Geo.topo --
   kwargs with `= None` default: 11
    2. topo(self, dem_raster, x_raster: Raster = None, y_raster: Raster = None,
       height_raster: Raster = None, incidence_angle_raster: Raster = None,
       heading_angle_raster: Raster = None, local_incidence_angle_raster: Raster = None,
       local_psi_raster: Raster = None, simulated_amplitude_raster: Raster = None,
       layover_shadow_raster: Raster = None, ground_to_sat_east_raster: Raster = None,
       ground_to_sat_north_raster: Raster = None) -> None

-- CUDA isce3.cuda.geometry.Rdr2Geo.topo --
   kwargs with `= None` default: 0
    2. topo(self, dem_raster, x_raster: Raster, y_raster: Raster,
       height_raster: Raster, incidence_angle_raster: Raster, heading_angle_raster: Raster,
       local_incidence_angle_raster: Raster, local_Psi_raster: Raster,  # <-- capital P
       simulated_amplitude_raster: Raster, layover_shadow_raster: Raster,
       ground_to_sat_east_raster: Raster, ground_to_sat_north_raster: Raster) -> None

Note the additional asymmetry: local_Psi_raster (capital P) on the CUDA
binding vs local_psi_raster (lowercase) on the CPU binding. Same root
cause as the missing defaults (see "Regression history" below).

Layer 2 — runtime call with kwarg subset (CPU passes, CUDA TypeError):

-- CPU  isce3.geometry.Rdr2Geo.topo(dem, x=..., y=..., z=...): PASS

-- CUDA isce3.cuda.geometry.Rdr2Geo.topo(dem, x=..., y=..., z=...): FAIL with TypeError
   topo(): incompatible function arguments. The following argument types are supported:
       1. (self, dem_raster, outdir: str) -> None
       2. (self, dem_raster, x_raster, y_raster, height_raster,
           incidence_angle_raster, heading_angle_raster,
           local_incidence_angle_raster, local_Psi_raster,
           simulated_amplitude_raster, layover_shadow_raster,
           ground_to_sat_east_raster, ground_to_sat_north_raster) -> None

   Invoked with: <Rdr2Geo>, <Raster>; kwargs: x_raster=<Raster>,
                 y_raster=<Raster>, height_raster=<Raster>

The same script run against a build with the proposed fix applied (see
"Proposed fix" below) shows CPU defaults=11/PASS, CUDA defaults=11/PASS
— behavior restored to symmetric.

Regression history

The CUDA binding originally had these defaults; they were lost in a later
patch.

• Commit
  a19944c0 "Optional topo layers" (2022-01)
  added = nullptr defaults symmetrically to both the CPU and CUDA
  Rdr2Geo.topo pybind bindings. The C++ class Topo::topo() was at the
  same time updated to take Raster* (not Raster&) for all raster args.
  This is the original, intentional optional-rasters design.
• Commit
  362813e7 "Add ability to compute/output East and North component for
  ground-to-satellite vector in Topo" (2023-07)
  extended both bindings with 2 new raster args. On the CPU side, the
  diff is in-place insertion — every existing = nullptr line untouched,
  +2 new lines. On the CUDA side, the binding block was rewritten in a
  different formatting style ((&Topo::topo) moved to its own line, the
  kwargs reflowed) and in the rewrite all = nullptr defaults were
  silently dropped. The same commit also renamed local_Psi_raster →
  local_psi_raster on the CPU side but left the capital-P version on
  CUDA. Both asymmetries share the same root cause: rewrite-vs-edit on
  the two siblings.

#868 and #1393 in those commit messages are JPL-internal MCR numbers
(GitLab), not GitHub PRs — they don't resolve via gh pr view.

The C++ class signature in
cxx/isce3/cuda/geometry/Topo.h:43-54
still takes Raster* per the original design. So this is purely a
binding-layer regression; the kernel and the C++ contract are unchanged.

Why existing tests didn't catch it

tests/python/extensions/pybind/cuda/geometry/rdr2geo.py::test_run_raster_layers
exercises the multi-raster overload but always passes every raster
kwarg explicitly. No test ever exercised the optional path on the CUDA
side, so CI was green throughout the #1393 change.

A test that would have caught the regression at the time of #1393 is
the same kind of test that ships in the proposed fix below
(test_run_optional_layers).

There is also an internal-callers point worth one line: NISAR's own
callers in nisar/workflows/{rdr2geo,geocode_corrections}.py pass all 12
raster args positionally with the unwanted ones as None. pybind11
converts None → nullptr for Raster* arguments regardless of whether
the binding declares a default. So that pattern is insensitive to the
missing defaults — only kwarg-subset callers (external consumers, or
hypothetical new tests) hit it.

Proposed fix

Two commits on a fork branch,
s-sasaki-earthsea-wizard/isce3@feat/cuda-rdr2geo-optional-kwargs:

1. 3638c4b2
   — Restore = nullptr defaults to CUDA Rdr2Geo.topo() optional rasters.
   Adds = nullptr to the 11 raster kwargs in
   python/extensions/pybind_isce3/cuda/geometry/rdr2geo.cpp to mirror the
   CPU sibling. Also renames local_Psi_raster → local_psi_raster for
   parity with the CPU binding and the docstring.
2. 185905d4
   — Add CUDA Rdr2Geo.topo() optional-rasters regression test. Adds
   test_run_optional_layers to
   tests/python/extensions/pybind/cuda/geometry/rdr2geo.py exercising the
   kwarg-subset call shape (a subset of layers requested, others passed as
   None or omitted).

Diff is ~15 lines of binding code + a regression test. After this patch,
pytest tests/python/extensions/pybind/cuda/geometry/rdr2geo.py -v
passes 5/5 (existing 4 + the new one).

Filing this as an issue first per project conventions; happy to open a PR
against develop if the approach matches what maintainers would prefer.

Reproducing

A minimal repro that imports only isce3 (no NISAR workflows, no
Sentinel-1, no external consumers) is at:

• script:
  scripts/repro_cuda_rdr2geo_optional_kwargs.py
• BEFORE artifact (origin/develop build):
  artifacts/before_repro_cuda_rdr2geo_develop.txt
• AFTER artifact (fork-branch build):
  artifacts/after_repro_cuda_rdr2geo_fork.txt

Companion report with the discovery context and the bench-shaped
verification (REF CSLC re-runs against a real S1 IW3 burst pair, 18/18
runs exit 0 after the fix vs TypeError on first GPU dispatch before):

• reports/2026-05-rfc-cuda-rdr2geo-optional-kwargs.md

[s-sasaki-earthsea-wizard]

Opened a PR for this: #270 (rebased onto current develop, no conflicts, pre-commit.ci passing).