Skip to content

Restore = nullptr defaults to CUDA Rdr2Geo.topo() optional rasters (#265)#270

Open
s-sasaki-earthsea-wizard wants to merge 2 commits into
isce-framework:developfrom
s-sasaki-earthsea-wizard:feat/cuda-rdr2geo-optional-kwargs
Open

Restore = nullptr defaults to CUDA Rdr2Geo.topo() optional rasters (#265)#270
s-sasaki-earthsea-wizard wants to merge 2 commits into
isce-framework:developfrom
s-sasaki-earthsea-wizard:feat/cuda-rdr2geo-optional-kwargs

Conversation

@s-sasaki-earthsea-wizard
Copy link
Copy Markdown

Closes #265.

Summary

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 with a subset of layers (kwargs only, others omitted) raised
TypeError on argument matching:

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. Full evidence — including pybind11 docstring inspection and a
self-contained repro that imports only isce3 — is in #265.

What's in this PR

Two commits:

  1. Restore = nullptr defaults to CUDA Rdr2Geo.topo() optional rasters
    — adds = nullptr to the 11 output-raster kwargs in
    python/extensions/pybind_isce3/cuda/geometry/rdr2geo.cpp to mirror
    the CPU sibling and the C++ class signature
    (cxx/isce3/cuda/geometry/Topo.h:43-54). Also renames
    local_Psi_rasterlocal_psi_raster for parity with the CPU
    binding (the lowercase form was already used by the CPU sibling and
    by the shared docstring; the capital-P was a leftover from the same
    rewrite that dropped the defaults).
  2. Add CUDA Rdr2Geo.topo() optional-rasters regression test
    test_run_optional_layers in
    tests/python/extensions/pybind/cuda/geometry/rdr2geo.py exercises
    the kwarg-subset call shape (subset of layers requested, others
    passed as None or omitted) so future binding-layer regressions on
    the optional path get caught. Also gates the whole module with
    pytestmark = pytest.mark.skipif on isce3.cuda availability so
    CPU-only builds skip cleanly with a reason rather than failing
    fixture collection.

Net diff: ~15 lines of binding code (11 = nullptr + the rename) plus a
regression test. No kernel changes, no C++ class changes — purely a
binding-layer restoration.

Why this is regression-shaped, not a feature

The defaults existed historically. The "Regression history" section in
#265 traces this: commit a19944c0 (2022, "Optional topo layers") added
= nullptr to both the CPU and CUDA bindings symmetrically. The
later commit 362813e7 (2023, "ground-to-sat East/North components")
rewrote the CUDA binding block in a different formatting style and
silently dropped every = nullptr along the way, while the CPU sibling
was edited in place and kept its defaults. This PR restores the
pre-362813e7 contract for the CUDA side.

Why existing CI 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 through 362813e7. The new
test_run_optional_layers covers the gap.

Internal NISAR callers (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 those callers are insensitive
to the missing defaults. Only kwarg-subset callers hit it.

Tests

pytest tests/python/extensions/pybind/cuda/geometry/rdr2geo.py -v
5/5 PASS (existing 4 + the new test_run_optional_layers) on a
CUDA-enabled build. On a CPU-only build the whole module reports a
clean skip via pytestmark.

The standalone isce3-only repro in #265 ("Reproducing" section) goes
from CPU defaults=11/PASS, CUDA defaults=0/TypeError (BEFORE) to
CPU defaults=11/PASS, CUDA defaults=11/PASS (AFTER) on this branch.

Notes

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.
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

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

1 participant