Run float-predictor encode through an ngjit row loop#1556
Merged
brendancol merged 2 commits intomainfrom May 11, 2026
Merged
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR optimizes the GeoTIFF floating-point predictor (TIFF predictor=3) encode path by moving the per-row dispatch loop into a single Numba-compiled wrapper, matching the already-optimized decode path. It also adds regression tests to ensure the refactor remains lossless and to provide an opt-in timing guard.
Changes:
- Add
@ngjit_fp_predictor_encode_rowsto run the predictor=3 row loop inside Numba and call it fromfp_predictor_encode. - Add a large (1024x1024) value-exact round-trip test for predictor=3 with deflate compression.
- Add a skipped timing regression test intended for manual performance checks.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
xrspatial/geotiff/_compression.py |
Moves predictor=3 encode row dispatch into an @ngjit wrapper to remove Python per-row overhead. |
xrspatial/geotiff/tests/test_predictor_fp_write_1313.py |
Adds a large lossless round-trip test and a manual timing regression test for predictor=3 encoding. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- The "bit-for-bit equal" claim now actually checks bits: assert dtype matches and compare ``arr.tobytes() == out_arr.tobytes()`` so the test catches signed-zero drift, NaN payload changes, and any other bit-level divergence that ``np.testing.assert_array_equal`` would silently mask. - Convert the timing test from an unconditional ``@pytest.mark.skip`` to the existing ``XRSPATIAL_RUN_PERF_TESTS=1`` env-var gate used by ``test_streaming_write_parallel.test_streaming_write_perf_sanity``. Developers can now run it with one env var instead of editing the decorator.
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
Closes #1554.
The predictor=3 (floating-point) encode path in
_compression.pywas calling its per-row kernel from a Pythonforloop, paying numba dispatch overhead on every row. The decode path was already moved into a single@ngjitwrapper (_fp_predictor_decode_rows); this does the same on the encode side by adding_fp_predictor_encode_rows. The per-row kernel is left in place so the change is additive.Numbers
1024x1024 float32 deflate write (single thread, this machine):
Predictor=3 was 2.5x slower than predictor=2. It is now a hair faster.
Verification
e573bccaa356d6cf7355976680c86387cd7c277ce6329c616273e3c4cbf5388fboth before and after the change. Output bytes are identical for the same input.pytest xrspatial/geotiff/tests/ -k "predictor or fp_predictor or float"runs 151 passed, 1 skipped (the new timing test).pytest xrspatial/geotiff/tests/runs 933 passed, 7 skipped (1 new), 7 deselected. The 7 deselected are TestPalette matplotlib deepcopy failures unrelated to this change.Test plan