Skip to content

Fix Fletcher32.decode out keyword and test#449

Merged
martindurant merged 1 commit intozarr-developers:mainfrom
mkitti:mkitti/fletcher32_out
Jul 20, 2023
Merged

Fix Fletcher32.decode out keyword and test#449
martindurant merged 1 commit intozarr-developers:mainfrom
mkitti:mkitti/fletcher32_out

Conversation

@mkitti
Copy link
Contributor

@mkitti mkitti commented Jul 18, 2023 •
edited
Loading

Fix #448 , enable out keyword of Fletcher32.decode.

  • 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 18, 2023 •
edited
Loading

The keyword out was not tested. I realized this from looking at coverage of jenkins lookup3 checksum.

@mkitti mkitti mentioned this pull request Jul 18, 2023
Merged
7 tasks
@mkitti
Copy link
Contributor Author

mkitti commented Jul 18, 2023

I am unclear if release notes are needed since Fletcher32 has not been released

@mkitti mkitti changed the title Fix Fletcher32 out and test Fix Fletcher32.decode out keyword and test Jul 18, 2023
@mkitti
Copy link
Contributor Author

mkitti commented Jul 19, 2023

@martindurant was this out keyword actually supposed to be used? The other thought I had was perhaps this is primarily vestigal and could just be removed.

@martindurant
Copy link
Member

martindurant commented Jul 19, 2023

Yes, I think out should be supported. I think zarr provides it when reading a whole chunk (rather than a slice) AND there are no further stages of decompression/decoding. The latter condition probably doesn't happen regularly in HDF5, which is where we find fletcher32, but it is possible.

@codecov
Copy link

codecov bot commented Jul 19, 2023 •
edited
Loading

Codecov Report

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

@@            Coverage Diff            @@
##              main      #449   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           55        55           
  Lines         2121      2126    +5     
=========================================
+ Hits          2121      2126    +5
Impacted Files Coverage Δ
numcodecs/tests/test_fletcher32.py 100.00% <100.00%> (ø)

@mkitti
Copy link
Contributor Author

mkitti commented Jul 20, 2023

Let me know if you think this is the correct fix or not then.

@martindurant martindurant merged commit f1d7c0e into zarr-developers:main Jul 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Fletcher32 decode out keyword does not work

3 participants

@mkitti @martindurant @dstansby