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 crc32c codec #613

Merged
merged 15 commits into from
Oct 28, 2024
Merged

Adds crc32c codec #613

merged 15 commits into from
Oct 28, 2024

Conversation

normanrz
Copy link
Contributor

@normanrz normanrz commented Oct 20, 2024

Adds Crc32c codec, which appends a crc32c checksum to the input buffer when encoding and validates the checksum when decoding.

Fixes #610

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 self-assigned this Oct 20, 2024
Copy link

codecov bot commented Oct 21, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.91%. Comparing base (88464f6) to head (e133d0b).
Report is 1 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #613   +/-   ##
=======================================
  Coverage   99.91%   99.91%           
=======================================
  Files          59       59           
  Lines        2326     2405   +79     
=======================================
+ Hits         2324     2403   +79     
  Misses          2        2           
Files with missing lines Coverage Δ
numcodecs/__init__.py 100.00% <100.00%> (ø)
numcodecs/checksum32.py 100.00% <100.00%> (ø)
numcodecs/tests/test_checksum32.py 100.00% <100.00%> (ø)

@normanrz normanrz requested a review from dstansby October 21, 2024 08:22
@dstansby
Copy link
Contributor

Should this live in https://github.com/zarr-developers/numcodecs/blob/main/numcodecs/checksum32.py, where we already have CRC32? As I understand (from a stackoverflow post) CRC32 and CRC32C are basically the same algorithm, so it makes sense to put them in the same namespace.

@normanrz
Copy link
Contributor Author

I didn't look for checksum32.py. Thanks for telling me.
I just looked into that. A problem is that the existing checksum codecs prepend the checksum whereas zarr3 expects the checksum to be appended. Either we single out crc32c to append the checksum or we add an option to all checksum codecs. Wdyt?

@normanrz
Copy link
Contributor Author

I moved the code to checksum32.py and refactored the tests (they had no parameterization) in 7e78d93

Copy link
Contributor

@dstansby dstansby left a comment

Choose a reason for hiding this comment

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

Looks good to me - just a few questions, but none of them blocking.

docs/checksum32.rst Show resolved Hide resolved
numcodecs/checksum32.py Show resolved Hide resolved
numcodecs/checksum32.py Show resolved Hide resolved
pyproject.toml Outdated Show resolved Hide resolved
@normanrz normanrz enabled auto-merge (squash) October 25, 2024 19:02
@normanrz
Copy link
Contributor Author

I don't see why this PR isn't ready to merge (it might be just late)? Do I need another approval?

@normanrz normanrz requested a review from dstansby October 28, 2024 09:06
Copy link
Contributor

@dstansby dstansby left a comment

Choose a reason for hiding this comment

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

Maybe just needs a fresh approval?

@dstansby dstansby disabled auto-merge October 28, 2024 10:54
@dstansby dstansby merged commit 230c52c into main Oct 28, 2024
53 checks passed
@dstansby dstansby deleted the crc32c branch October 28, 2024 10:54
@jakirkham jakirkham mentioned this pull request Nov 14, 2024
7 tasks
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.

Add crc32c codec
2 participants