Add clevel argument for Zstd #164
Conversation
|
Thanks for the PR! |
|
tests should be passing now. |
|
Could someone approve the CI workflow so that it runs in full again? |
|
done |
|
Great. Thank you. Looks like all the CI passes now as well. |
|
One issue I encountered in testing is the inability to access negative levels:
|
There was a problem hiding this comment.
I find an issue with the ZSTD HDF5 filter which clips the clevel to [1, 22]:
https://github.com/aparamon/HDF5Plugin-Zstandard/blob/d5afdb5f04116d5c2d1a869dc9c7c0c72832b143/zstd_h5plugin.c#L38-L39
Since negative values are not currently supported, there are a minor updates to do.
Co-authored-by: Thomas VINCENT <thomas.vincent@esrf.fr>
src/hdf5plugin/test.py
Outdated
| {'clevel': 0}, | ||
| {'clevel': -1}, | ||
| {'clevel': -1000} |
There was a problem hiding this comment.
Should we still test that 0 and negative values "work"?
There was a problem hiding this comment.
No need, an assert on the clevel value would be best.
src/hdf5plugin/test.py
Outdated
| {'clevel': 0}, | ||
| {'clevel': -1}, | ||
| {'clevel': -1000} |
There was a problem hiding this comment.
No need, an assert on the clevel value would be best.
src/hdf5plugin/__init__.py
Outdated
| """ | ||
| filter_id = ZSTD_ID | ||
|
|
||
| def __init__(self, clevel=None): | ||
| if clevel is not None: |
There was a problem hiding this comment.
An assert on clevel would be good: For the other filters we have assert on the input arguments when possible. We do so since error messages from hdf5 can be hard to understand for end-users.
Example for blosc: https://github.com/silx-kit/hdf5plugin/blob/main/src/hdf5plugin/__init__.py#L201
| if clevel is not None: | |
| if clevel is not None: | |
| assert 1 <= clevel <= 22 |
Co-authored-by: Thomas VINCENT <thomas.vincent@esrf.fr>
Fix #163