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

Change ArrayV3Metadata.data_type to DataType #2278

Merged
merged 2 commits into from
Oct 4, 2024

Conversation

rabernat
Copy link
Contributor

@rabernat rabernat commented Oct 1, 2024

In working on #2036, I realized that the current treatment of the data_type attribute in V3 metadata was somewhat inconsistent. This class is meant to represent the Zarr metadata in an abstract way, in alignment with the spec. However, it was just using numpy dtypes internally.

This becomes a problem when we want to support dtypes that are not numpy dtypes, like string.

This PR changes the ArrayV3Metadata class such that the type of data_type is the existing DataType enum. However, I've kept the .dtype property returning a numpy dtype. This allows this change to be minimally disruptive to other parts of the code.

@rabernat rabernat requested a review from d-v-b October 1, 2024 01:41
@rabernat rabernat changed the title Change v3.metadata.data_type to DataType Change ArrayV3Metadata.data_type to DataType Oct 1, 2024
@rabernat rabernat requested a review from jhamman October 1, 2024 01:55
@@ -490,8 +492,11 @@ def to_numpy_shortname(self) -> str:
}
return data_type_to_numpy[self]

def to_numpy_dtype(self) -> np.dtype[Any]:
Copy link
Contributor

Choose a reason for hiding this comment

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

I could see to_numpy as a slightly briefer name for this method

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I kind of prefer it more explicit, but I suppose that information is in the type annotation, so ok.

@classmethod
def from_dtype(cls, dtype: np.dtype[Any]) -> DataType:
def from_numpy_dtype(cls, dtype: np.dtype[Any]) -> DataType:
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

modulo to/from of course

src/zarr/core/metadata/v3.py Outdated Show resolved Hide resolved
@rabernat rabernat requested a review from d-v-b October 4, 2024 13:02
@rabernat rabernat merged commit 01346dd into zarr-developers:v3 Oct 4, 2024
20 checks passed
@jhamman jhamman added the V3 label Oct 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants