Refactor GeoTIFF Phase 5f: extract _encode.py from _writer.py#2263
Merged
Conversation
brendancol
commented
May 21, 2026
Contributor
Author
brendancol
left a comment
There was a problem hiding this comment.
PR Review: Refactor GeoTIFF Phase 5f: extract _encode.py from _writer.py
Blockers (must fix before merge)
None.
Suggestions (should fix, not blocking)
-
_encode.pyline 27-29 (module docstring): the docstring says heavier helpers are looked up lazily inside_compress_block/_prepare_strip/_prepare_tile, but the lazy_writerimport is actually in_write_stripped(line 369) and_write_tiled(line 516)._compress_block,_prepare_strip, and_prepare_tilethemselves do no lazy lookup. Tighten the wording to point at the actual call sites.
Nits (optional improvements)
- The lazy-lookup comments at
_encode.py:365-368and_encode.py:511-515are clear, but consider adding a one-line cross-reference to the test that pins the contract (test_gil_friendly_kwarg_1830::test_write_tiled_parallel_passes_gil_friendly_positionally), so the next refactor knows which test will catch a regression.
What looks good
- Move is mechanical and byte-neutral. Every helper and constant lands in the same shape it had in
_writer.py. - Re-export block at
_writer.py:97-114preserves the historical import surface for the_writerssubpackage,_gpu_decode, and the test suite. - Lazy
_writer._prepare_tile/_writer._prepare_striplookup inside_write_tiledand_write_strippedkeeps the monkeypatch contract pinned bytest_gil_friendly_kwarg_1830working without test changes. Same pattern_write_layout.pyuses for_resolve_photometric. - No module-level cycle:
_encode.pyonly pulls_compression,_header, and_write_layoutat load time;_writeris imported lazily inside the two orchestrators that need the monkeypatch indirection. _writer.pyline count: 1800 to 1269 (-531). Inside the issue's ~1300 target.- Module docstring follows
_decode.py's style and calls out the symmetry with PR-G.
Checklist
- Byte parity preserved (no algorithmic changes)
- Public import paths preserved (
xrspatial.geotiff._writer.<name>resolves for every moved name) - Monkeypatch contract preserved (
test_gil_friendly_kwarg_1830,test_streaming_write_parallel,test_write_layout_monkeypatch_contract_2248all still pass) - Full geotiff test suite passes (5037 passed; one lz4 failure is pre-existing on
main, not caused by this PR) - No new public API
- No README / docs touch needed (private refactor)
brendancol
commented
May 21, 2026
Contributor
Author
brendancol
left a comment
There was a problem hiding this comment.
PR Review (second pass): Refactor GeoTIFF Phase 5f: extract _encode.py from _writer.py
Blockers (must fix before merge)
None.
Suggestions (should fix, not blocking)
None remaining. Prior pass's docstring suggestion was applied at _encode.py:23-32.
Nits (optional improvements)
None remaining. Prior pass's cross-reference nit was applied at _encode.py:365-370 (strip path) and _encode.py:511-518 (tile path, with the full test node-id).
What looks good
- Follow-up commit is documentation-only; behaviour unchanged.
- Module docstring now names
_write_strippedand_write_tiledas the two lazy-lookup sites instead of pointing at the three prepare/compress leaves. - Both lazy-lookup comments now name the pinning test by id, so a future refactor that inlines the indirection will know which test will catch it.
- Full geotiff test suite still passes the way it did on the first commit (5011 passed excluding the pre-existing lz4 byte-parity failure on
main).
Checklist
- Byte parity preserved
- Public import paths preserved
- Monkeypatch contracts preserved (
test_gil_friendly_kwarg_1830,test_streaming_write_parallel,test_write_layout_monkeypatch_contract_2248) - Full geotiff test suite passes (modulo the pre-existing lz4 failure)
- No new public API
- Documentation matches actual call sites
Clean. No further changes requested.
Move the strip / tile encode helpers, the photometric resolution and MinIsWhite inversion helpers, the predictor normalisation, the ``_compression_tag`` mapping, and the streaming ``_compress_block`` helper out of ``_writer.py`` into a new ``_encode.py``. Mirrors the ``_decode.py`` extraction on the read side (PR-G). ``_writer.py`` re-exports every moved name so the existing public import path (``xrspatial.geotiff._writer.<name>``) and downstream backends (``_writers/eager.py``, ``_writers/gpu.py``, ``_writers/vrt.py``, ``_write_layout.py``, the test suite) keep working without churn. ``_write_tiled`` and ``_write_stripped`` look up ``_prepare_tile`` and ``_prepare_strip`` through ``_writer`` so the monkeypatch contract pinned by ``test_gil_friendly_kwarg_1830`` survives the move. ``_writer.py`` drops from 1800 to 1269 lines. Part of #2211.
…2260) Tighten the module docstring to point at the actual call sites where the lazy _writer import lives (_write_stripped / _write_tiled), and cross-reference the test that pins the monkeypatch contract from the inline comments next to those lookups. Pure documentation; no behaviour change.
d913b1d to
95fe6a8
Compare
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.
Closes #2260
Part of #2211
Summary
Move the write-side encode helpers out of
_writer.pyinto a new_encode.py. Mirrors the_decode.pyextraction on the read side(PR-G).
Helpers relocated:
_prepare_strip,_write_stripped,_prepare_tile,_write_tiled_compress_block_resolve_photometric,_reject_disagreeing_photometric_override,_apply_photometric_miniswhite_invert,_invert_nodata_for_miniswhite,PHOTOMETRIC_MINISBLACK/PHOTOMETRIC_RGB/_PHOTOMETRIC_NAME_MAPnormalize_predictor,_apply_predictor_encode_compression_tag_PARALLEL_MIN_BYTES_writer.pykeeps_write,_write_streaming,_validate_lowlevel_write_kwargs, and the fsspec write helpers, andre-exports every moved name so existing import paths and the
_writerssubpackage continue to work without churn._write_tiledand_write_strippedlook up_prepare_tileand_prepare_stripthrough_writerso the monkeypatch contractpinned by
test_gil_friendly_kwarg_1830survives the move._writer.pydrops from 1800 to 1269 lines (~531 line move).Test plan
pytest xrspatial/geotiff/tests/-- 5037 passed, 68 skipped,one pre-existing lz4 byte-parity failure unrelated to this PR.
tag tests pass unchanged.
from xrspatial.geotiff._writer import _resolve_photometricetc.) still resolves.