geotiff: writer pre-inverts MinIsWhite pixels so values round-trip (#1836)#1837
Open
brendancol wants to merge 1 commit into
Open
geotiff: writer pre-inverts MinIsWhite pixels so values round-trip (#1836)#1837brendancol wants to merge 1 commit into
brendancol wants to merge 1 commit into
Conversation
…1836) photometric='miniswhite' set the TIFF Photometric tag to 0 without inverting pixel values. The reader unconditionally inverts single-band MinIsWhite data (see _reader._apply_photometric_miniswhite), so to_geotiff(..., photometric='miniswhite') followed by open_geotiff returned iinfo(dtype).max - original (uint) or -original (float) instead of the user's values. Mirror the reader's inversion on the writer side so the round trip is the identity. Invert the nodata sentinel alongside the pixels so the reader's existing mask logic (issue #1809) identifies the correct nodata positions and rewrites them to NaN. Signed integer data passes through unchanged on both sides. Reject photometric='miniswhite' with cog=True or explicit overview_levels: the overview reducers ('min', 'max', 'mode', ...) do not commute with the pixel inversion, so summary statistics would not match the user-domain values. Mean would still work in principle, but the codepath does not know which reducer the user picked at the point where the inversion would have to be undone, so refuse the combination outright with a clear error. Update test_miniswhite_backend_parity_1797 to assert the new (correct) round-trip identity. The previous expectation pinned the buggy sign-flip; the HTTP cases keep their existing expectations since tifffile-written files do not pre-invert.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR updates GeoTIFF MinIsWhite writing so single-band data round-trips consistently with the reader’s existing MinIsWhite inversion behavior.
Changes:
- Adds eager writer-side MinIsWhite pixel and nodata pre-inversion.
- Rejects MinIsWhite writes with COG/overview generation in the eager writer path.
- Adds and updates regression tests for MinIsWhite round-trip behavior.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
xrspatial/geotiff/_writer.py |
Adds MinIsWhite inversion helpers and applies them in eager write(). |
xrspatial/geotiff/tests/test_miniswhite_backend_parity_1797.py |
Updates backend parity expectation for writer-produced MinIsWhite files. |
xrspatial/geotiff/tests/test_miniswhite_writer_roundtrip_1836.py |
Adds regression coverage for MinIsWhite writer round-trips and rejection cases. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+1547
to
+1549
| _samples = data.shape[2] if data.ndim == 3 else 1 | ||
| _resolved_photo, _ = _resolve_photometric(photometric, _samples) | ||
| if _resolved_photo == 0 and _samples == 1: |
| path = tmp_path / 'ov_msw_1836.tif' | ||
| with pytest.raises(NotImplementedError, match='miniswhite'): | ||
| to_geotiff(_da(arr), str(path), photometric='miniswhite', | ||
| cog=True, overview_levels=[2, 4]) |
Comment on lines
+113
to
+114
| non-finite / fractional sentinels, mirroring the reader exactly. | ||
| See issue #1836. |
| ``_reader._apply_photometric_miniswhite``. Before issue #1836 the writer | ||
| set the TIFF Photometric tag to 0 without inverting pixel values, so | ||
| ``to_geotiff(..., photometric='miniswhite')`` followed by ``open_geotiff`` | ||
| returned ``iinfo(dtype).max - original`` instead of the user's values. |
Comment on lines
+1558
to
+1561
| data = _apply_photometric_miniswhite_invert( | ||
| data, _resolved_photo, _samples) | ||
| if nodata is not None: | ||
| nodata = _invert_nodata_for_miniswhite(nodata, data.dtype) |
| info = np.iinfo(dtype) | ||
| if not (info.min <= vi <= info.max): | ||
| return nodata | ||
| return info.max - vi |
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.
Fixes #1836.
Summary
to_geotiff(..., photometric='miniswhite')set the TIFF Photometric tag to 0 without inverting pixel values. The reader unconditionally inverts single-band MinIsWhite data, so the round trip returnediinfo(dtype).max - original(uint) or-original(float). For analytical rasters this was silent data corruption.photometric='miniswhite'withcog=Trueor explicitoverview_levels: overview reducers (min,max,mode, ...) do not commute with the pixel inversion, so the on-disk summary statistics would not match the user-domain values.test_miniswhite_backend_parity_1797to assert the new round-trip identity. The previous expectation pinned the sign-flip bug; HTTP-served files keep their expectations becausetifffiledoes not pre-invert.Test plan
test_miniswhite_writer_roundtrip_1836.pycover uint8 / uint16 / float32 round-trip, signed-int passthrough, nodata-via-NaN round-trip, and thecog/overview_levelsrefusals.xrspatial/geotiff/tests/run reproduce onmain(pre-existing GPU andtest_featuresissues, unrelated to this PR).