issue_comments

@kmuehlbauer thanks for adding tests! I'm not sure what the mypy error is either, I'm afraid...

So it looks like the changes here with the fix in my branch will get your issue resolved @tomwhite, right?

Yes - thanks!

I'm a bit worried, that this might break other users workflows, if they depend on the current conversion to floating point for some reason.

The floating point default is preserved if you do e.g. xr.Dataset({"a": np.array([], dtype=object)}). The change here will only convert to string if there is extra metadata present that says it is a string.

Could you verify the above example, please?

The code looks fine, and I get the same result when I run it with this PR.

Your fix in https://github.com/kmuehlbauer/xarray/tree/preserve-vlen-string-dtype changes the metadata so it is correctly preserved as metadata: {'element_type': <class 'str'>}.

I feel less qualified to evaluate the impact of the netcdf4 fix.

Thanks for taking a look @kmuehlbauer and for the useful example code. I hadn't considered the netcdf cases, so thanks for pointing those out.

Engine netcdf4 does not roundtrip here, losing the dtype metadata information. There is special casing for h5netcdf backend, though.

Could netcdf4 do the same special-casing as h5netcdf?

Thanks for all your hard work on this @TomNicholas!

Ah I understand better now. This makes sense - if ChunkManager has a name then the implementation could use that to name tasks.

If you hover over a node in the SVG representation you'll get a tooltip that shows the call stack and the line number of the top-level user function that invoked the computation. Does that help at all? (That said, I'm open to changing the way it is displayed, or how tasks are named in general.)

BTW should this be moved to a cubed issue?

Related issue: https://github.com/data-apis/array-api/issues/621

I would like to get to the point where you can use xarray with a chunked array without ever importing dask. I think this PR gets very close, but that would be tricky to test because cubed depends on dask (so I can't just run the test suite without dask in the environment

I just released Cubed 0.6.0 which doesn't have a dependency on Dask, so this should be possible now.

I was hoping https://github.com/data-apis/array-api-compat might help with this, but I'm not sure it does...

I think it might make sense for me to remove the CubedManager class from this PR and instead put that & cubed+xarray tests into another repo. That keeps xarray's changes minimal, doesn't require putting cubed in any xarray CI envs, and hopefully allows us to merge the ChunkManager changes here earlier.

That sounds like a good plan to me.

Places dask is still explicitly imported in xarray

There are a few remaining places where I haven't generalised to remove specific import dask calls either because it won't be imported at runtime unless you ask for it, cubed doesn't implement the equivalent function, that function isn't in the array API standard, or because I'm not sure if the dask concept used generalises to other parallel frameworks.

• [ ] open_mfdataset(..., parallel=True) - there is no cubed.delayed to wrap the open_dataset calls in,
• [ ] Dataset.__dask_graph__ and all the other similar dask magic methods
• [ ] dask_array_ops.rolling - uses functions from dask.array.overlap,
• [ ] dask_array_ops.least_squares - uses dask.array.apply_along_axis and dask.array.linalg.lstsq,
• [ ] dask_array_ops.push - uses dask.array.reductions.cumreduction

This is a useful list! I hope that we could close the gap for some of these over time.

I would like to get to the point where you can use xarray with a chunked array without ever importing dask. I think this PR gets very close, but that would be tricky to test because cubed depends on dask (so I can't just run the test suite without dask in the environment)

Agreed. I have opened https://github.com/tomwhite/cubed/issues/154 to make it possible to test without a Dask dependency.

Great work @TomNicholas!

I don't have a strong opinion about the tests, but putting them in a new project to keep xarray changes to a minimum is probably a good idea for the moment.

Thanks for the review @Illviljan

This behaviour stems from this part of _infer_dtype where empty object arrays are converted to float arrays:

https://github.com/pydata/xarray/blob/3aa75c8d00a4a2d4acf10d80f76b937cadb666b7/xarray/conventions.py#L156-L157

Is there any reason we couldn't return strings.create_vlen_dtype(str) instead?

You can work around this by specifying either decode_cf=False or mask_and_scale=False. For example:

python ds = xarray.open_zarr("test.zarr", consolidated=False, decode_cf=False) ds['array'].dtype

prints dtype('int8').

Does that help?

Cubed implements concat, but perhaps xarray needs richer concat functionality than that?

@TomNicholas it might be good to rebase this now that #7067 is in.

Updated with all the suggestions. How does this look now?

This is great feedback - thanks @keewis! I will take a look at implementing your suggestions.

I think the manager keyword will also need adding to open_zarr, open_dataset and to_zarr.

I'm interested in trying this out on some of our genomics use cases in sgkit (see https://github.com/pystatgen/sgkit/issues/908), so please let me know when you think it's ready to try @TomNicholas.

I think it can't be tested with pytorch until they compete pytorch/pytorch#58743, right?

It needs __array_namespace__ to be defined to activate the new code path.

Hi @hsharrison - thanks for offering to do some testing. Here's a little demo script that you could try, by switching numpy.array_api to pytorch: https://github.com/tomwhite/xarray/commit/929812a12818ffaa1187eb860c9b61e3fc03973c

Thanks for opening this @TomNicholas

The challenge will be defining a parallel computing API that works across all these projects, with their slightly different models.

Agreed. I feel like there's already an implicit set of "chunked array" methods that xarray expects from Dask that could be formalised a bit and exposed as an integration point.

Opened #6804

I started having a look at making xarray work with the array API here: https://github.com/tomwhite/xarray/commit/c72a1c4a4c52152bdab83f60f35615de28e8be7f. Some basic operations work (preserving the underlying array): https://github.com/tomwhite/xarray/commit/929812a12818ffaa1187eb860c9b61e3fc03973c. If there's interest, I'd be happy to turn this into a PR with some tests.

Thanks for confirming @jhamman

This is now passing. We could add more tests or even change the representation a little in some cases, but it would be good to merge for the release (since it fixes a bug) if it looks OK to you @keewis.

Thank you!

@max-sixty yes - I've rebased and updated whats-new.

Thanks for the review @jsignell!

Thanks for the suggestions @dcherian!

I've implemented "default" to preserve existing behaviour (i.e. only collapse if greater than preconfigured size limits). See #5126

I haven't implemented the nested options for display_expand - is it worth adding the extra complexity here?

I would find it useful to have a generalized version of this for datasets, so it's possible to control independently whether the coordinates, data variables, and attributes sections are expanded by default.

I'd be happy to pick this up if no one else is working on it. I have created a branch to try it out, which adds the options display_expand_attrs, display_expand_coords, display_expand_data, and display_expand_data_vars for controlling both HTML and text representations. All default to True.

One thing I'm not sure about is maintaining the existing behaviour, which is that for the HTML dataset repr all three sections are expanded by default, up to a certain size limit, depending on the section (25 coordinates, 15 data variables, 10 attributes). I couldn't see a simple way to incorporate that, although maybe it's not needed if the sections can be collapsed by setting an option (e.g. xr.set_options(display_expand_attrs=False))?