Skip to content

Updated Zstd plugin; Added support for negative compression levels to Zstd#395

Merged
t20100 merged 6 commits into
silx-kit:mainfrom
t20100:update-zstd-upstream
Jun 23, 2026
Merged

Updated Zstd plugin; Added support for negative compression levels to Zstd#395
t20100 merged 6 commits into
silx-kit:mainfrom
t20100:update-zstd-upstream

Conversation

@t20100

@t20100 t20100 commented Jun 23, 2026

Copy link
Copy Markdown
Member

! Merge PR #394 first !

This PR changes upstream project for the Zstd filter from the initial repository (ttps://github.com/aparamon/HDF5Plugin-Zstandard) to the version hosted by the hdfgroup organisation (https://github.com/HDFGroup/hdf5_plugins).

The reason is that the former project is stalled while the later is actively maintained.
For instance, support for negative compression levels (favor speed over compression ratio) as initially proposed in #165 is implemented in the hdfgroup's version of the filter.

This PR also adds support for negative compression levels in hdf5plugin.Zstd class.

@t20100 t20100 added this to the 7.0.0 milestone Jun 23, 2026
Comment thread src/hdf5plugin/_filters.py Outdated
raise ValueError(
f"clevel must be in the range [{self._ZSTD_MIN_CLEVEL}, {self._ZSTD_MAX_CLEVEL}]"
)
clevel_uint32 = struct.unpack("I", struct.pack("i", clevel))[0]

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Need to pass uint32. The filter cast this back to int

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

And we are agree that it is cast the same way ?

clevel = -12
clevel_uint32 = struct.unpack("I", struct.pack("i", clevel))[0]
struct.unpack("i", struct.pack("I", clevel_uint32))[0]
>> -12

Could we add a comment on this. Because indeed this is a bit tricky and better to highlight that this is done on purpose...

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I hesitated to make helper functions to make it more explicit.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

To be honest I saw the reverse function a couple of line after but I let the comment.

This is super simple I agree but I would be in favor of helper function (This is easier to test and to associate).

@t20100 t20100 Jun 23, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Added _to_uint32 and _from_uint32 helpers in 58afed3

@t20100 t20100 requested a review from payno June 23, 2026 08:30
@t20100 t20100 changed the title Updated zstd plugin Updated zstd plugin; Added support for negative compression levels to Zstd Jun 23, 2026
@t20100 t20100 changed the title Updated zstd plugin; Added support for negative compression levels to Zstd Updated Zstd plugin; Added support for negative compression levels to Zstd Jun 23, 2026
@t20100 t20100 force-pushed the update-zstd-upstream branch from e594958 to 1b83200 Compare June 23, 2026 12:55
Comment thread src/hdf5plugin/_filters.py Outdated
raise ValueError(
f"clevel must be in the range [{self._ZSTD_MIN_CLEVEL}, {self._ZSTD_MAX_CLEVEL}]"
)
clevel_uint32 = struct.unpack("I", struct.pack("i", clevel))[0]

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

And we are agree that it is cast the same way ?

clevel = -12
clevel_uint32 = struct.unpack("I", struct.pack("i", clevel))[0]
struct.unpack("i", struct.pack("I", clevel_uint32))[0]
>> -12

Could we add a comment on this. Because indeed this is a bit tricky and better to highlight that this is done on purpose...

@t20100 t20100 force-pushed the update-zstd-upstream branch from 1b83200 to 58afed3 Compare June 23, 2026 14:43
@t20100 t20100 merged commit 5e389b0 into silx-kit:main Jun 23, 2026
4 checks passed
@t20100 t20100 deleted the update-zstd-upstream branch June 23, 2026 14:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants