geotiff: route write_vrt dtype through central resolver#1919
Merged
Conversation
write_vrt built the VRT dataType attribute from a local if/elif/else ladder keyed on sample_format and bps. The ladder had no entry for bps=12, so unsigned 12-bit TIFF sources were tagged as Byte in the VRT header. The reader path promotes those samples to uint16, so downstream GDAL/VRT consumers could truncate values above 255. Replace the ladder with _vrt_dtype_name_for, which calls tiff_dtype_to_numpy and then looks up the GDAL name in the existing _NP_TO_VRT_DTYPE table. A numpy dtype with no GDAL mapping now raises ValueError instead of falling back to Byte. The helper is exposed at module scope so the dtype path can be unit-tested without writing a 12-bit TIFF to disk.
Contributor
There was a problem hiding this comment.
Pull request overview
Replaces a local dtype ladder in write_vrt with a helper that funnels TIFF bps/sample_format through the central tiff_dtype_to_numpy resolver, fixing the case where 12-bit unsigned sources were incorrectly tagged as Byte in the VRT.
Changes:
- Add
_vrt_dtype_name_for(bps, sample_format)helper in_vrt.pythat resolves viatiff_dtype_to_numpyand maps through_NP_TO_VRT_DTYPE, raisingValueErroron unsupported types. - Replace the if/elif/else dtype ladder in
write_vrtwith a single helper call. - Add regression tests covering all supported
(bps, sample_format)pairs and end-to-endwrite_vrtruns onuint16/int16sources.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| xrspatial/geotiff/_vrt.py | Introduces _vrt_dtype_name_for helper and routes write_vrt dtype resolution through it. |
| xrspatial/geotiff/tests/test_vrt_dtype_12bit_1914.py | Adds regression and end-to-end tests for VRT dtype mapping including the 12-bit case. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes #1914.
write_vrtbuilt the VRTdataTypeattribute from a local if/elif/else ladder keyed onsample_formatandbps. Forbps=12, sf=1it fell through to'Byte', even though the reader path in_dtypes.pypromotes the same sample touint16. A VRT over a valid 12-bit unsigned source advertised a narrower type than the actual data, which can cause downstream GDAL/VRT readers to truncate values above 255.The ladder also defaulted to
'Byte'/'Int32'for any other unknown bps, silently mis-typing future bit widths.Changes
_vrt_dtype_name_for(bps, sample_format)toxrspatial/geotiff/_vrt.py. It callstiff_dtype_to_numpy(the central TIFF-to-numpy resolver) and looks up the GDAL name in the existing_NP_TO_VRT_DTYPEtable. A numpy dtype with no GDAL mapping raisesValueErrorinstead of falling back toByte.write_vrtwith a single call to the helper.xrspatial/geotiff/tests/test_vrt_dtype_12bit_1914.pycovering every supported(bps, sample_format)pair (sub-byte, 12-bit, 8/16/32/64-bit integer, 32/64-bit float), the regression case explicitly, and an end-to-end run viawrite_vrtonuint16andint16source TIFFs.Test plan
python -m pytest xrspatial/geotiff/tests/test_vrt_dtype_12bit_1914.py -x -q(20 passed)python -m pytest xrspatial/geotiff/tests/ -q -k "vrt or VRT"(all VRT tests pass; pre-existing GPU and tile-size failures onmainare unrelated)xrspatial/geotiff/