isce-framework / isce-framework/isce3
Regression: CUDA Rdr2Geo.topo() pybind binding lost optional-raster defaults
Nobody has claimed this yet.
- Dominant language
- Python
- Stars
- 239
- Forks
- 90
- Avg merge
- 13d 1h
- Merged PRs (30d)
- 5
Description
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= nullptrdefaults symmetrically to both the CPU and CUDA
Rdr2Geo.topopybind bindings. The C++ classTopo::topo()was at the
same time updated to takeRaster*(notRaster&) 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= nullptrline 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= nullptrdefaults were
silently dropped. The same commit also renamedlocal_Psi_raster→
local_psi_rasteron 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:
3638c4b2
—Restore = nullptr defaults to CUDA Rdr2Geo.topo() optional rasters.
Adds= nullptrto the 11 raster kwargs in
python/extensions/pybind_isce3/cuda/geometry/rdr2geo.cppto mirror the
CPU sibling. Also renameslocal_Psi_raster→local_psi_rasterfor
parity with the CPU binding and the docstring.185905d4
—Add CUDA Rdr2Geo.topo() optional-rasters regression test. Adds
test_run_optional_layersto
tests/python/extensions/pybind/cuda/geometry/rdr2geo.pyexercising the
kwarg-subset call shape (a subset of layers requested, others passed as
Noneor 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/developbuild):
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):
Contributor guide
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- Open a pull request that references the issue number.
Research direction
Start with python/extensions/pybind_isce3/cuda/geometry/rdr2geo.cpp and compare its topo() binding with the CPU sibling, then read tests/python/extensions/pybind/cuda/geometry/rdr2geo.py::test_run_raster_layers. Run pytest tests/python/extensions/pybind/cuda/geometry/rdr2geo.py -v; done means the optional-layer call works and all five tests pass with matching keyword names and defaults.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- cpp, python
- Domain
- backend-api-design
- Issue type
- Bug
- Difficulty
- 2/5
- Estimated time
- 1-3 hours
- Activity status
- Quiet
- Clarity
- Clearly specified
- Newbie friendliness
- 45/100