Fail loudly on unsupported GeoTIFF feature combinations (PR 5 of epic #2340)#2355
Conversation
Epic #2340 PR 5: detect unsupported GeoTIFF / VRT feature combinations at the entry point and raise an actionable typed error naming the feature and pointing at SUPPORTED_FEATURES. Cases newly rejected: * VRTDataset subClass attributes (VRTWarpedDataset, VRTPansharpenedDataset, etc.) rejected at parse time. The reader has no warp / pansharpen pipeline and silent dispatch on whatever simple sources happen to be embedded would drop the subclass semantics. * VRTRasterBand subClass attribute (VRTDerivedRasterBand etc.) rejected at parse time. No pixel-function evaluator exists. * Unknown VRTRasterBand child elements raise rather than silently skip. Known informational children (Description, UnitType, Offset, Scale, Metadata, ColorTable, ...) keep passing; known output-altering children (KernelFilteredSource, MaskBand, PansharpeningOptions, PixelFunction*, ...) and any unrecognised tag raise. * write_vrt now refuses sources whose declared transform carries a non-zero skew term (rotated_affine), refuses cross-source AREA_OR_POINT mismatch, and refuses cross-source nodata mismatch unless the caller pins the mosaic nodata via the nodata kwarg. New UnsupportedGeoTIFFFeatureError exported from xrspatial.geotiff. Subclasses ValueError so existing ``except ValueError`` callers keep catching the cases. Regression test suite covers each gate plus the existing rotated 6-tuple writer refusal and the rotated VRT GeoTransform read refusal so future refactors cannot regress them back to silent fallback. Backend coverage: numpy / dask+numpy / cupy / dask+cupy share the parse_vrt and write_vrt entry points; the gates run before the backend dispatch, so all four hit the same error.
brendancol
left a comment
brendancol
left a comment
There was a problem hiding this comment.
PR Review: Fail loudly on unsupported GeoTIFF feature combinations (PR 5 of epic #2340)
Blockers
None.
Suggestions
-
Dataset-level
<MaskBand>slips through. Per the GDAL VRT spec,<MaskBand>sits as a child of<VRTDataset>(sibling of<VRTRasterBand>), not inside a band. The scan at_vrt.py:504-534only walks band children, so a VRT carrying a dataset-level mask band still silently drops the mask. PuttingMaskBandin_UNSUPPORTED_BAND_TAGSdoes not help. Either sweep the dataset root forMaskBand/GCPListafter thesubClasscheck at_vrt.py:390, or note that the gap is deliberate. -
The three frozensets at
_vrt.py:490-502are rebuilt on every band iteration. Hoist them to module scope. -
<OverviewList>and<Overview>(band-level VRT overview declarations) are in none of the three sets. The new "raise on unknown" branch at_vrt.py:520-534will now reject a VRT that declares external overviews on a band -- the previous parser ignored those. If ignoring was intentional, add both tags to_INFORMATIONAL_BAND_TAGS. If rejecting is intentional, add a regression test.
Nits
-
The new write-side checks at
_vrt.py:1705-1828are interleaved with the legacy pixel-size / dtype / CRS checks. A helper or even a comment block grouping the new gates would make the cross-source policy easier to follow. Not a correctness issue. -
Error messages embed
:data:`xrspatial.geotiff.SUPPORTED_FEATURES`in raw RST. Callers see this in tracebacks, not in rendered Sphinx output, so the:data:role is dead weight. Plain`xrspatial.geotiff.SUPPORTED_FEATURES`reads better.
What looks good
- 13 tests cover each rejected case plus regression pins for the existing rotated-write and rotated-VRT-read paths.
UnsupportedGeoTIFFFeatureErrorsubclassesValueError, so existingexcept ValueErrorcallers keep catching the case.- The mixed-nodata gate has an explicit
nodata=opt-out and the test covers it. - Each new check has a comment naming the silent-failure mode it replaces.
- The rotated 6-tuple writer refusal and the rotated VRT GeoTransform read refusal both pick up regression pins.
Checklist
- VRT subClass attribute and GeoTransform skew terms match the GDAL VRT spec.
- All four backends share
parse_vrtandwrite_vrt, so the new gates hit uniformly. - No NaN-handling regressions; the new checks are metadata-only.
- Edge cases covered (asymmetric nodata, derived band subClass, unknown band child).
- No premature materialization; the gates are O(num_sources) attribute reads.
- [n/a] Benchmark not needed (validation-only, runs before IO).
- [n/a] README feature matrix unchanged (no new functions added).
- Docstring present on
UnsupportedGeoTIFFFeatureError.
…le (#2349) Review fixes for PR 5 of epic #2340: * Add dataset-level sweep for ``<MaskBand>``, ``<GCPList>``, and dataset-level ``<PansharpeningOptions>``. The band-children loop never saw these because they sit as siblings of ``<VRTRasterBand>``; without the new sweep a VRT carrying a dataset-level mask band silently dropped the mask. * Hoist the three band-children classification frozensets (``_INFORMATIONAL_BAND_TAGS`` / ``_SOURCE_BAND_TAGS`` / ``_UNSUPPORTED_BAND_TAGS``) to module scope so parse_vrt does not rebuild them on every band iteration. * Add ``OverviewList`` and ``Overview`` to the informational set so the new "raise on unknown band child" branch does not regress VRTs with band-level overview declarations -- the previous parser ignored these and the new gate now ignores them too, matching real-world GDAL-emitted VRTs. * Group the new write-side cross-source checks (rotated source transform, mixed AREA_OR_POINT registration, mixed nodata) into three module-level helpers (``_check_no_rotated_source_transforms`` / ``_check_no_mixed_raster_type`` / ``_check_no_mixed_nodata``) so the cross-source policy is easy to read end-to-end. The pre-#2349 pixel-size / dtype / band-count / CRS checks stay inline because they predate this PR. * Drop the ``:data:`` Sphinx role from error messages. Callers see these strings in tracebacks, not rendered docs, so the plain ``\`xrspatial.geotiff.SUPPORTED_FEATURES\``` reads better. Three new tests pin the dataset-level MaskBand / GCPList refusals and the OverviewList allow-list.
…2349) test_features.TestPublicAPI.test_all_lists_supported_functions pins the exact public surface of ``xrspatial.geotiff.__all__``. The new ``UnsupportedGeoTIFFFeatureError`` added in this PR's earlier commit needs to be in the expected set too.
``float('nan') != float('nan')`` evaluates True in plain Python, so
the cross-source nodata check would flag two perfectly consistent
NaN-sentinel sources as a mismatch. NaN nodata is the standard
sentinel for float32 / float64 GeoTIFFs, so this would have made
the new fail-closed gate too aggressive on real-world inputs.
Compare via ``math.isnan`` so two NaNs are equal. Add a regression
test that mosaics two sources both carrying NaN nodata and pins
that the writer no longer rejects them.
brendancol
left a comment
brendancol
left a comment
There was a problem hiding this comment.
Follow-up review (post-revision)
Re-reviewed after the address-review commits (510e515, 0ff6f06, 88e5904).
Disposition of prior findings
- Dataset-level
<MaskBand>/<GCPList>/<PansharpeningOptions>sweep -- fixed at_vrt.py:458-475. Two new regression tests pin the rejection. - Frozensets hoisted to module scope -- fixed at
_vrt.py:366-400. <OverviewList>and<Overview>band children kept informational -- fixed at_vrt.py:376. Regression test added.- Write-side checks grouped into helpers (
_check_no_rotated_source_transforms,_check_no_mixed_raster_type,_check_no_mixed_nodata) -- done at_vrt.py:1646-1749. :data:Sphinx role dropped from error messages -- done.
New findings from this pass
Self-caught while re-reviewing
Two sources both declaring NaN nodata would have hit the new mixed-nodata gate falsely because float('nan') != float('nan') is True in plain Python. Fixed in 88e5904 by routing the comparison through _nodata_values_agree (math.isnan-aware). Regression test pins the round-trip.
Blockers
None.
Suggestions
None remaining.
Nits
None remaining.
Verification
- 17 tests in
test_unsupported_features_2349.pypass. - Full
xrspatial/geotiff/tests/suite: 5358 passed, 68 skipped, 1 xfailed, 1 xpassed.
Ready for CI. No outstanding review items.
1 participant
Closes #2349. Parent epic #2340 (PR 5 of 6).
Summary
Surface a typed error at the entry point for the unsupported feature combinations listed in the release contract, instead of letting them slip through as silent no-ops or wrong output.
write_vrtrefuses sources with a rotated source transform, refuses cross-source AREA_OR_POINT mismatch, and refuses cross-source nodata mismatch unless the caller pins the mosaic nodata via thenodatakwarg.UnsupportedGeoTIFFFeatureErrorexported fromxrspatial.geotiff. SubclassesValueErrorso existingexcept ValueErrorcallers keep catching the case.SUPPORTED_FEATURESplus epic Epic: GeoTIFF release contract and feature tiering #2340.Out of scope
SUPPORTED_FEATURESreshape (PR 1).Backend coverage
parse_vrtandwrite_vrt; the new gates run before backend dispatch, so all four hit the same error.Test plan
xrspatial/geotiff/tests/test_unsupported_features_2349.pycover each rejected case plus the existing rotated 6-tuple writer refusal and the rotated VRT GeoTransform read refusal.-k vrt) still passes (709 tests).-k rotated) still passes (99 tests).