Conversation
jrbourbeau
left a comment
There was a problem hiding this comment.
Thanks @mpeaton, this generally looks good to me! I've attached a few comments. Also, it's my understanding that we want to make the PR for this fix from the Quansight-Labs Dask fork
- Could you please add a regression test to
dask/array/tests/test_array_core.pywhich fails onmasterand demonstrates this PR fixes thefrom_zarrnameissue - There are a couple of minor
flake8linting issues - There was a recent update to the main Dask
masterbranch which fixed a failingzarr-related test. Merging the current Daskmasterbranch into this PR will prevent that failing test from showing up here
dask/array/core.py
Outdated
| z = zarr.Array(mapper, read_only=True, path=component, **kwargs) | ||
| chunks = chunks if chunks is not None else z.chunks | ||
| return from_array(z, chunks, name='zarr-%s' % url) | ||
| return from_array(z, chunks, name = name) |
There was a problem hiding this comment.
It would be useful for the default name of the output array to indicate it was created with from_zarr. For example, something like
if name is None:
name = 'from-zarr-' + tokenize(z, component, storage_options, chunks, **kwargs)There was a problem hiding this comment.
I'm not sure I agree. The only reason I can think of for dissent however, is that establishing protocol for identifying source in a name string seems undesirable, as opposed to say, creating a higher level object to accumulate such information.
There was a problem hiding this comment.
creating a higher level object to accumulate such information
The name information here is stored in the dask graph representation of the dask array. Using more descriptive graph key names like 'from-zarr-...', while not required, can be useful when inspecting the underlying dask graph (especially when debugging) and is generally used throughout the codebase. For example:
https://github.com/dask/dask/blob/a3cba093ea041a999258e518ab5d5f5c93f611b1/dask/array/core.py#L2342
There was a problem hiding this comment.
I follow. Ok thanks. Change pushed.
* Add name kwarg to from_zarr * added default from_zarr naming convention per jrbourbeau * flake8 fix
* Add name kwarg to from_zarr * added default from_zarr naming convention per jrbourbeau * flake8 fix
* Add name kwarg to from_zarr * added default from_zarr naming convention per jrbourbeau * flake8 fix
* Add name kwarg to from_zarr * added default from_zarr naming convention per jrbourbeau * flake8 fix
flake8 dask