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

Use unsigned bytes to back Buffer #2738

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

Conversation

QuLogic
Copy link
Contributor

@QuLogic QuLogic commented Jan 21, 2025

This makes compressors consistent with v2, and seems more correct than signed bytes.

Fixes #2735

Note that this does not implement accepting both signed and unsigned, as there wasn't a conclusion in #2735 about that.

TODO:

  • Add unit tests and/or doctests in docstrings
  • Add docstrings and API docs for any new/modified user-facing classes and functions
  • New/modified features documented in docs/user-guide/*.rst
  • Changes documented in docs/release-notes.rst
  • GitHub Actions have all passed
  • Test coverage is 100% (Codecov passes)

@QuLogic
Copy link
Contributor Author

QuLogic commented Jan 21, 2025

PS, going to ignore tests for now.

This makes compressors consistent with v2, and buffers consistents with
`bytes` types.

Fixes zarr-developers#2735
@github-actions github-actions bot added the needs release notes Automatically applied to PRs which haven't added release notes label Jan 25, 2025
@dstansby
Copy link
Contributor

I just fixed uploading code coverage for the GPU tests at #2767, so hopefully when I merge in main here it should improve the patch coverage 🤞

@maxrjones
Copy link
Member

maxrjones commented Apr 11, 2025

Hi @QuLogic and @dstansby, thanks for working on this! Could I help at all with resolving the codecov issue? I'd be super excited to see this completed to enable further trying out imagecodecs with Zarr V3 (e.g., maxrjones/virtual-tiff#4).

@jakirkham
Copy link
Member

(clicking update branch in case that fixes it)

@jakirkham
Copy link
Member

Closing and reopening to hopefully fix RTD (it has a git checkout error)

@jakirkham jakirkham closed this Apr 11, 2025
@jakirkham jakirkham reopened this Apr 11, 2025
@jakirkham
Copy link
Member

Ok better RTD checked out the code 😌

@jakirkham
Copy link
Member

Looks like Codecov got stuck somewhere. Let's try another close/reopen

@jakirkham jakirkham closed this Apr 11, 2025
@jakirkham jakirkham reopened this Apr 11, 2025
@maxrjones
Copy link
Member

I don't think that the codecov/patch target will be hit unless the overall coverage actually increases, rather than remains constant. The issue is that two of the lines changed (L87 and L110) are not tested in main either - https://app.codecov.io/gh/zarr-developers/zarr-python/blob/main/src%2Fzarr%2Fcore%2Fbuffer%2Fgpu.py, so the PR isn't decreasing code coverage but still will not hit the 80% target for code coverage for patches (i.e., all new/modified lines of code). Do you prefer to ignore the codecov/patch target or to include unit tests for adding and creating zero length buffers?

@jakirkham
Copy link
Member

If someone can add some tests, that would be welcome

What was concerning to me was one Codecov job says "failing after 1s". Though this may be a non-issue and just the report percentage (as you suggested)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs release notes Automatically applied to PRs which haven't added release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Buffer uses signed bytes with v2 compressors
5 participants