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.

Sign up for GitHub

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

Jump to bottom

base64 encode fill value for some dtypes with zarr_format=2 #2286

Merged
merged 7 commits into from
Oct 7, 2024
Merged

base64 encode fill value for some dtypes with zarr_format=2 #2286

merged 7 commits into from
Oct 7, 2024

Conversation

TomAugspurger
Copy link
Contributor

@TomAugspurger TomAugspurger commented Oct 1, 2024 •
edited
Loading

Closes #2282

Needs more tests.

  • 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/tutorial.rst
  • Changes documented in docs/release.rst
  • GitHub Actions have all passed
  • Test coverage is 100% (Codecov passes)

@TomAugspurger TomAugspurger mentioned this pull request Oct 2, 2024
Closed
4 tasks
@TomAugspurger TomAugspurger marked this pull request as ready for review October 2, 2024 19:40
@TomAugspurger
Copy link
Contributor Author

TomAugspurger commented Oct 2, 2024 •
edited
Loading

This should be ready to go (the CI failure is from appveyor).

@TomAugspurger TomAugspurger changed the title [WIP]: base64 encode fill value for some dtypes with zarr_format=2 base64 encode fill value for some dtypes with zarr_format=2 Oct 2, 2024
@d-v-b
Copy link
Contributor

d-v-b commented Oct 2, 2024

were we not doing this in the v2 codebase? I couldn't find code for it.

@TomAugspurger
Copy link
Contributor Author

I found it at

zarr-python/zarr/meta.py

Line 215 in 6b9ab9e

def decode_fill_value(cls, v: Any, dtype: np.dtype, object_codec: Any = None) -> Any:
(and borrowed the implementation).

d-v-b
d-v-b reviewed Oct 4, 2024
View reviewed changes
src/zarr/core/metadata/v2.py Outdated Show resolved Hide resolved
@jakirkham
Copy link
Member

Thanks Tom! 🙏

JFYI we have a Base64 codec in Numcodecs, perhaps it is worth using to align on behavior

@TomAugspurger
Copy link
Contributor Author

Thanks John. I think this version (rather than numcodec's) is OK. It ends up using the same actual implementation of base64.standard_b64encode but it's slightly more direct with the calls directly in the serialization code in this PR.

@TomAugspurger TomAugspurger merged commit cc50b41 into zarr-developers:v3 Oct 7, 2024
20 checks passed
@TomAugspurger TomAugspurger deleted the tom/fix/v2-encode-decode-fill-value branch October 7, 2024 19:04
@rabernat rabernat mentioned this pull request Oct 9, 2024
Open
@jhamman jhamman added the V3 Affects the v3 branch label Oct 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@d-v-b d-v-b d-v-b left review comments

@rabernat rabernat rabernat approved these changes

Assignees
No one assigned
Labels
V3 Affects the v3 branch
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Fix fill_value encoding for fixed-width string / binary v2 array
5 participants
@TomAugspurger @d-v-b @jakirkham @rabernat @jhamman