-
Notifications
You must be signed in to change notification settings - Fork 12
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
update zarrita, relaxes numpy requirement #932
Conversation
normanrz
commented
Jul 31, 2023
- Updates zarrita, relaxes numpy requirement
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks already good to me. I think it would be nice to have more consistent use of chunk_shape
and shard_shape
.
@@ -867,6 +867,7 @@ def test_chunking_wk(data_format: DataFormat, output_path: Path) -> None: | |||
ds_path = prepare_dataset_path(data_format, output_path) | |||
ds = Dataset(ds_path, voxel_size=(2, 2, 1)) | |||
chunk_shape, chunks_per_shard = default_chunk_config(data_format, 8) | |||
shard_shape = chunk_shape * chunks_per_shard |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the naming of shard_shape
and chunk_shape
is confusing. Why is the chunk_shape
for initializing the mag different from the chunk_shape
for iterating over the mag?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that is a bit unfortunate. shard and chunk are the terms of the Zarr storage, but chunk is also used for the iterator functions. Do you have an idea how to resolve that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me. Regarding the naming issue with chunk_shape
and shard_shape
I don't have a better idea for the variable names.