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

Add jenkins lookup3 32-bit checksum #446

Merged
merged 8 commits into from
Jul 19, 2023

Conversation

mkitti
Copy link
Contributor

@mkitti mkitti commented Jul 13, 2023

Add Bob Jenkin's lookup3 from HDF5 via a Cython implementation

Fix #445

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)

@mkitti
Copy link
Contributor Author

mkitti commented Jul 13, 2023

@rabernat Here's a cython implementation of jenkin's lookup3 32-bit checksum.

@mkitti
Copy link
Contributor Author

mkitti commented Jul 13, 2023

I'm thinking of moving initval into the codec so that it is defined at the constructor.

Also, I'm thinking about a prepend value in case we need to add extra bytes to the beginning of the bytes being checksummed.

numcodecs/jenkins.pyx Outdated Show resolved Hide resolved
numcodecs/jenkins.pyx Outdated Show resolved Hide resolved
numcodecs/jenkins.pyx Outdated Show resolved Hide resolved
numcodecs/jenkins.pyx Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Jul 13, 2023

Codecov Report

Merging #446 (f579b00) into main (4f194b6) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##              main      #446    +/-   ##
==========================================
  Coverage   100.00%   100.00%            
==========================================
  Files           55        56     +1     
  Lines         2121      2238   +117     
==========================================
+ Hits          2121      2238   +117     
Impacted Files Coverage Δ
numcodecs/__init__.py 100.00% <100.00%> (ø)
numcodecs/checksum32.py 100.00% <100.00%> (ø)
numcodecs/tests/test_jenkins.py 100.00% <100.00%> (ø)

@martindurant
Copy link
Member

test_jenkins.py is showing lots of lint errors

@mkitti
Copy link
Contributor Author

mkitti commented Jul 14, 2023

Could someone run CI?

@martindurant
Copy link
Member

@mkitti
Copy link
Contributor Author

mkitti commented Jul 15, 2023

Flake8 issues resolved

@mkitti
Copy link
Contributor Author

mkitti commented Jul 16, 2023

I made some changes to docs/Makefile. When I ran docs/Makefile without having sphinx on my path, I got the following cryptic Makefile error:

$ ~/src/numcodecs/docs$ make
Makefile:12: *** recipe commences before first target.  Stop.

This was due to the if statement near the top of the file detecting the lack of sphinx, and then proceeding to try to print the "friendly" error message. However, it accomplishes exactly the opposite.

@mkitti
Copy link
Contributor Author

mkitti commented Jul 16, 2023

I modified fletcher32 since the out keyword was not working in my hands. I also added a test for it.

@mkitti
Copy link
Contributor Author

mkitti commented Jul 16, 2023

Let me know if you vwould prefer me to break this up a bit. I originally intended that but slipped in a commit -a by accident.

@martindurant
Copy link
Member

Yes, please keep only the jenkins stuff in here. The other bits look OK I think, but it's better to keep PRs focussed where possible.

@mkitti
Copy link
Contributor Author

mkitti commented Jul 18, 2023

I split off unrelated changes into #449 (Fletcher32) and #451 (sphinx Makefile)

@martindurant martindurant merged commit 63e820e into zarr-developers:main Jul 19, 2023
23 checks passed
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 Jenkin's lookup3 as a 32-bit checksum for HDF5
2 participants