Skip to content

Drop lz4 from byte-parity sweep, gated as experimental (#2268)#2269

Merged
brendancol merged 1 commit into
mainfrom
issue-2268
May 21, 2026
Merged

Drop lz4 from byte-parity sweep, gated as experimental (#2268)#2269
brendancol merged 1 commit into
mainfrom
issue-2268

Conversation

@brendancol
Copy link
Copy Markdown
Contributor

Summary

  • xrspatial/geotiff/tests/test_lowlevel_write_pushdown_2138.py::test_write_vs_to_geotiff_byte_parity_uint8[lz4] was failing because _write / to_geotiff now reject lz4 at the experimental-codec gate from geotiff: tier the public surface; mark experimental codecs/features opt-in #2137 unless the caller passes allow_experimental_codecs=True.
  • Remove lz4 from _PARITY_CODECS. The other experimental codecs (lerc, jpeg2000, j2k) were never in the tuple, so the result matches the docstring's "stable lossless codec set" framing.
  • Note the experimental-codec exclusion in the tuple's leading comment so the next maintainer to add a codec sees the gate up front.

Closes #2268.

Test plan

  • pytest xrspatial/geotiff/tests/test_lowlevel_write_pushdown_2138.py -v -- 26 passed (was 26 passed, 1 failed before the change)

`lz4` moved into `_EXPERIMENTAL_CODECS` via #2137, so direct callers
must now pass `allow_experimental_codecs=True`. The parity sweep in
`test_lowlevel_write_pushdown_2138.py` calls `_write` and `to_geotiff`
without the opt-in and was failing at the gate.

The other experimental codecs (`lerc`, `jpeg2000`, `j2k`) are already
absent from `_PARITY_CODECS`. Removing `lz4` keeps the list consistent
with the docstring's "stable lossless codec set" framing.

Closes #2268.
@github-actions github-actions Bot added the performance PR touches performance-sensitive code label May 21, 2026
Copy link
Copy Markdown
Contributor Author

@brendancol brendancol left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR Review: Drop lz4 from byte-parity sweep, gated as experimental (#2268)

Blockers (must fix before merge)

  • (none)

Suggestions (should fix, not blocking)

  • (none)

Nits (optional improvements)

  • xrspatial/geotiff/tests/test_lowlevel_write_pushdown_2138.py:46-51: the _codec_available("lz4") branch is now dead code in this file. Nothing else in the module exercises lz4 after the parity-tuple change. You can drop the lz4 branch from _codec_available or leave it as a generic skip-probe for future experimental-opt-in tests. Either is defensible.

What looks good

  • Diff is exactly what the issue calls for: one tuple entry removed, with a comment explaining the experimental-codec gate cited to issue #2137.
  • The exclusion matches the existing treatment of lerc, jpeg2000, and j2k, which were never in the parity tuple. The list is now internally consistent.
  • The remaining 26 parametrizations are stable codecs that round-trip losslessly, so the parity contract the test was written to defend is preserved.
  • No production code touched; no test downgraded.

Checklist

  • Algorithm matches reference/paper -- n/a, test-only change
  • All implemented backends produce consistent results -- n/a
  • NaN handling is correct -- n/a
  • Edge cases are covered by tests -- existing coverage preserved
  • Dask chunk boundaries handled correctly -- n/a
  • No premature materialization or unnecessary copies -- n/a
  • Benchmark exists or is not needed -- not applicable
  • README feature matrix updated (if applicable) -- not applicable
  • Docstrings present and accurate -- comment updated to cite #2137 gate

@brendancol brendancol merged commit 3834466 into main May 21, 2026
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

performance PR touches performance-sensitive code

Projects

None yet

Development

Successfully merging this pull request may close these issues.

test_write_vs_to_geotiff_byte_parity_uint8[lz4] fails after experimental-codec gate (#2137)

1 participant