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

Zarr v3: support root path #1085

Merged
merged 11 commits into from
Sep 8, 2022

Conversation

grlee77
Copy link
Contributor

@grlee77 grlee77 commented Jul 19, 2022

closes #1039

v3 spec states path = '/' for arrays gives an array at /meta/root.array.json
path = '/' for groups gives a group at /meta/root.array.json

In this implementation path=None will also result in a root array. Creation routines default to path=None, so this makes it so that the path argument does not have to be manually specified.

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

grlee77 added 7 commits July 15, 2022 11:43
v3 spec states path = '/' for arrays gives /meta/root.array.json
               path = '/' for groups gives /meta/root.array.json

In this implementation path = None or path = '' will also result in
a root array. Creation routines default to path=None, so this makes
it so that the path argument does not have to be manually specified.
update test_nbytes_stored to handle both v3 and v2 cases properly
@grlee77 grlee77 added the bug Potential issues with the zarr-python library label Jul 19, 2022
@grlee77 grlee77 changed the title V3 support root path Zarr v3: support root path Jul 19, 2022
@codecov
Copy link

codecov bot commented Jul 19, 2022

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.94%. Comparing base (ece1810) to head (e8c23c8).
Report is 344 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1085   +/-   ##
=======================================
  Coverage   99.94%   99.94%           
=======================================
  Files          34       34           
  Lines       13846    13853    +7     
=======================================
+ Hits        13839    13846    +7     
  Misses          7        7           
Files Coverage Δ
zarr/_storage/store.py 100.00% <100.00%> (ø)
zarr/creation.py 100.00% <100.00%> (ø)
zarr/hierarchy.py 99.78% <ø> (-0.01%) ⬇️
zarr/storage.py 100.00% <100.00%> (ø)
zarr/tests/test_convenience.py 100.00% <ø> (ø)
zarr/tests/test_core.py 100.00% <100.00%> (ø)
zarr/tests/test_creation.py 100.00% <100.00%> (ø)
zarr/tests/test_hierarchy.py 100.00% <100.00%> (ø)

@joshmoore
Copy link
Member

@grlee77, thanks for this! Reading over it, it does seem to address the issues that Stephan raised. Do you have anything else for this PR from your side or just need testing and then rolling into a 2.12.x release??

@grlee77
Copy link
Contributor Author

grlee77 commented Jul 20, 2022

I think test coverage is pretty good now. It should be ready for review.

@joshmoore
Copy link
Member

Rolling into a 2.13 pre-release for better testing together with xarray.

cc: @shoyer

@joshmoore joshmoore merged commit 2cfee9c into zarr-developers:main Sep 8, 2022
@joshmoore joshmoore mentioned this pull request Sep 8, 2022
6 tasks
grlee77 added a commit to grlee77/zarr-python that referenced this pull request Sep 21, 2022
path=None support was previously added in zarr-developersgh-1085. This change should have
been made at that time.
joshmoore pushed a commit that referenced this pull request Sep 22, 2022
path=None support was previously added in gh-1085. This change should have
been made at that time.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Potential issues with the zarr-python library
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Why is path required for opening Zarr v3 groups?
2 participants