issue_comments

Thanks - awesome! Even going through the code took ages so kudos for sticking with it!

I fixed a conflict and am merging.

Thanks @toddrjen ! This is a very significant contribution.

Thank you very much @toddrjen — it's a huge contribution!

All tests now pass as well.

The version here should be complete, in that all planned features are implemented, although of course there may be additional changes. So I removed the [WIP] part and updated whats-new.rst and others. I squashed my commits down and force-pushed to get a clean look at things. Please take a look and tell me what you think.

The latest version I just pushed should have the requested changes. It also has cat, join, +, *, %. I have also implemented broadcasting for many (but not all) of the functions I plan to implement it for so you can see some examples of how it works.

api.rst contains a structured enumeration of methods autosummary should generate documentation pages for, so there's not much to do, and it would allow us to check the rendered version of the docstrings.

@keewis Thanks for the suggestions. I will add everything to the relevant documentation when I have everything completed and the changes are agreed upon.

I opened an issue regarding the dtypes check. Let's see what the others think.

@mathause One possibility might be to make xr.testing.assert_identical match dtypes. I can see different dtypes being "equal", but not "identical".

Sorry for the delay, I have been swamped at work. I probably won't have any time to work on this before Christmas.

No probem at all, just wanted to check in.

Yes you are correct concerning the dtype. This comes back to numpy, where the following returns true

python np.array("a") == np.array("a", dtype=object) I wonder if that's the right choice... Thus, you are right to change the tests.

@mathause

Sorry for the delay, I have been swamped at work. I probably won't have any time to work on this before Christmas.

I have finished implementing the cat and join methods, and I implemented +, *, and % operator support.

I am currently working on improving the vectorization of some of the functions. The idea is that some arguments, like for example the regular expression pattern or the number of repetitions in rep, will be able to be given an array-like, with the dimensions being broadcast against the original DataArray.

This can be useful, for example, if a DataArray combines data of different formats along a dimension (ideally this wouldn't be the case but people don't always have that much control over the data they get). Or it could be used to create an ASCII bar chart where the number of symbols is equal to the value in an array element.

However, this could lead to conflicts if the DataArray already has a dimension with that name, which would be a particular problem if people chained together multiple such operations.

That should raise a KeyError, no?

Yes, but I think it would be strange if using the default parameters once works fine, but using them twice or more in a row somehow returns an exception. I think the defaults should either work generally or not be defaults at all. That is just my opinion. More fundamentally, it is just inconsistent with how xarray works elsewhere and so I think it would be unexpected.

• Some of the tests could probably be simplified, to make them easier to read. E.g. when you try to raise an error.

Please point out the specific cases if you haven't already done so.

• we usually add a match to the pytest.raises. This also helps to understand what you are testing.

I will add this.

• assert_equal should raise an error if the dtype does not match, so you should not need to add all the assert result.dtype == expected.dtype.

It doesn't work with an object dtype:

```python

import numpy as np import xarray as xr

a = xr.DataArray(np.array("a", dtype=np.str_)) b = a.astype(np.object_)
a.dtype == b.dtype
False a.equals(b) True xr.testing.assert_equal(a, b) ```

This does not raise an exception on my machine at least. I ran into several cases where I was incorrectly getting object dtypes and the tests weren't catching it, hence the dtype checks.

Just wanted to let you know that we are definitively interested in this contribution! I'd start by removing the dtype assertions again, that makes the diff of the tests much smaller and more digestible. Unless there is a reason for them? Does this not work correctly in assert_equal?

• I'd set a default for the name of new dimensions e.g. group_dim: Hashable = "group". I think that's a good choice in most cases.

I thought about doing this at first. However, this could lead to conflicts if the DataArray already has a dimension with that name, which would be a particular problem if people chained together multiple such operations. So I checked what default name xarray uses elsewhere, and it doesn't seem to use default names for the most part (the main exception being DataArray creation). So I think that, in order to avoid unexpected behavior, and to keep consistency, not automatically choosing a name is a better option.