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

Manifest arrays use arrayv3metadata #429

Merged
merged 81 commits into from
Feb 18, 2025
Merged

Manifest arrays use arrayv3metadata #429

merged 81 commits into from
Feb 18, 2025

Conversation

abarciauskas-bgse
Copy link
Collaborator

@abarciauskas-bgse abarciauskas-bgse commented Feb 6, 2025 •
edited
Loading

This is still very much a WIP - many tests and implementations still need to be fixed.

A few notes:

  • It was suggested we remove ZArray completely as a part of this work, as opposed to using a conversion function for ZArrays to ArrayV3Metadata. So we should be able to remove ZArray as a part of this pr.
  • It was suggested not to use zarr's _parse_chunk_encoding_v3 function since it is a private function and may change, which is why some of that logic is replicated in convert_to_codec_pipeline

Checklist

  • Closes ManifestArray should use zarr-python's ArrayV3Metadata #424
  • Manifest tests passing
  • Library (codecs, etc) tests passing
  • Reader tests passing
  • test_integration tests passing
  • test_xarray tests passing
  • Writer tests passing
  • Cleanup dead code
  • Consider reorganizing codecs and zarr modules
  • Full type hint coverage
  • Tests added for new functions
  • Changes are documented in docs/releases.rst
  • New functions/methods are listed in api.rst
  • New functionality has documentation

@abarciauskas-bgse abarciauskas-bgse mentioned this pull request Feb 6, 2025
Closed
TomNicholas
TomNicholas reviewed Feb 6, 2025
View reviewed changes
@TomNicholas TomNicholas added zarr-python Relevant to zarr-python upstream internals labels Feb 6, 2025
abarciauskas-bgse
abarciauskas-bgse commented Feb 15, 2025
View reviewed changes
conftest.py
@@ -1,13 +1,25 @@
"""Pytest configuration and fixtures for virtualizarr tests."""
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I reorganized this file as it includes fixtures, variables and helper functions of various purpose.

abarciauskas-bgse
abarciauskas-bgse commented Feb 15, 2025
View reviewed changes
virtualizarr/readers/hdf/hdf.py
@@ -283,9 +284,6 @@ def _dataset_to_variable(
list: xarray.Variable
A list of xarray variables.
"""
# This chunk determination logic mirrors zarr-python's create
# https://github.com/zarr-developers/zarr-python/blob/main/zarr/creation.py#L62-L66
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I removed this comment because I think the reference is from a previous version of zarr-python - @sharkinsspatial do you know if we can and should include an updated link?

@abarciauskas-bgse
Copy link
Collaborator Author

@TomNicholas this PR is ready for re-review and 🤞🏽 hopefully good to merge (to zarr-python-3.0). There are a few outstanding comments that are mostly related to creating new issues:

And then I might also create a ticket to test this branch against all (or most) current examples.

@TomNicholas TomNicholas mentioned this pull request Feb 18, 2025
Open
TomNicholas
TomNicholas approved these changes Feb 18, 2025
View reviewed changes
Copy link
Member

@TomNicholas TomNicholas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @abarciauskas-bgse ! Let's just get this merged into a stable branch so that others can build on it.

@abarciauskas-bgse abarciauskas-bgse merged commit e9a4cce into zarr-python-3.0 Feb 18, 2025
2 checks passed
@norlandrhagen
Copy link
Collaborator

Awesome work @abarciauskas-bgse!

This was referenced Feb 24, 2025
Closed
Closed
This was referenced Feb 25, 2025
Closed
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sharkinsspatial sharkinsspatial sharkinsspatial left review comments

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

@TomNicholas TomNicholas TomNicholas approved these changes

Assignees

@abarciauskas-bgse abarciauskas-bgse

Labels
internals zarr-python Relevant to zarr-python upstream
Projects
VirtualiZarr Zarr-Python 3.0 migration
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

ManifestArray should use zarr-python's ArrayV3Metadata
5 participants
@abarciauskas-bgse @TomNicholas @norlandrhagen @sharkinsspatial @d-v-b