Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adds checksum flag to zstd codec #519

Open
wants to merge 10 commits into
base: main
Choose a base branch
from
Open

Adds checksum flag to zstd codec #519

wants to merge 10 commits into from

Conversation

normanrz
Copy link

@normanrz normanrz commented Apr 22, 2024

This PR adds the checksum flag to the zstd codec. This is necessary to support the proposed zstd codec for Zarr3. We need it for the v3 refactoring of zarr-python.

TODO:

  • Unit tests and/or doctests in docstrings
  • Tests pass locally
  • Docstrings and API docs for any new/modified user-facing classes and functions
  • Changes documented in docs/release.rst
  • Docs build locally
  • GitHub Actions CI passes
  • Test coverage to 100% (Codecov passes)

@normanrz normanrz changed the title Adds checksum flag to zsdt codec Adds checksum flag to zstd codec Apr 22, 2024
@normanrz
Copy link
Author

normanrz commented May 8, 2024

I would love a review on this so that we can ship this in the upcoming zarr-python 3 release.

@d-v-b
Copy link
Contributor

d-v-b commented May 8, 2024

@mkitti would you mind giving this a look?

@martindurant
Copy link
Member

A good time to switch to cramjam? Does this zstd provide anything that that one doesn't?

@normanrz
Copy link
Author

normanrz commented May 8, 2024

A good time to switch to cramjam? Does this zstd provide anything that that one doesn't?

cramjam also does not expose the checksum option.

@mkitti
Copy link
Contributor

mkitti commented May 8, 2024

I'm looking. We should normalize the implementation here such that negative compression levels are passed on to the C library and that the default compression level is the default compression level of the underlying C library.

@mkitti
Copy link
Contributor

mkitti commented May 8, 2024

The default CLEVEL here should be changed to 0. That should be passed on the C library directly without further logic.

DEFAULT_CLEVEL = 0

@normanrz
Copy link
Author

normanrz commented May 8, 2024

The default CLEVEL here should be changed to 0. That should be passed on the C library directly without further logic.

DEFAULT_CLEVEL = 0

I made that change. This is a breaking change, though.

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.

None yet

4 participants