issue_comments

@shoyer I like that suggestion a lot - not sure why we didn't think of something similar before.

How does it work if I push to a merged PR?

:tada: Thanks for your help!

Okay I've changed those things. @shoyer if you want to just double check that the new error catching behaviour in dimension_coords_exist() is what you wanted, then this should be good to go!

Thanks for the detailed review @shoyer !

I'm loving all the documentation and test coverage here (though I haven't read them all carefully yet!).

Great - the only test I think you should definitely look at is test_manual_concat_too_many_dims_at_once, and my diagnosis of the issue it exposes in #2975.

Thanks for the review - I've fixed those mistakes, and added a note explaining why there are 3 similar-sounding functions.

Ah yes that worked, thanks @dcherian !

I think this PR is finally complete - ready to merge @shoyer?

@dcherian I added another section to the https://xarray.pydata.org/en/stable/combining.html docs page to explain what the new functions do.

One thing - this deprecation cycle means that for now warnings get thrown basically however you use auto_combine. The test suites are generating a lot of these warnings - is there a way to get pytest to ignore specific warnings for specific tests or is it okay leaving it?

@dcherian the relevant docstrings have been significantly altered, including giving some examples.

However I haven't changed any of the main documentation pages, I guess I should look through to see if this will change any of the syntax in any of those examples.

Or are you suggesting there should be a new section of the documentation explaining what the difference between combine_manual and combine_auto is?

@shoyer I think this is ready for a final review.

We can deprecate the old auto_combine but keep it around for now to enable a smooth transition.

This might be a little bit confusing until the deprecation cycle is complete, but it will allow us to complete the cycle in only one edition, rather than two (one to introduce combine_manual, and one to change the behaviour of auto_combine). It also means that users who want to eventually use the "coordinate-ordering" behaviour will only have to change their code once.

I'll do it this way.

I think that manual_combine and auto_combine (or combine_auto etc.) are pretty suitable.

Any variation on concat is a little misleading because these functions will also merge. Anything with nested or multi is kind of superfluous when the idea of the library is sort of that multi-dimensional operations are as easy as single-dimensional operations.

combine (dictionary definition: to join or merge to form a single unit or substance) does accurate describe what the functions do.

If we use combine_auto and combine_manual then it simplifies the deprecation cycle because we can have the old auto_combine and the new combine_auto in the API simultaneously (though maybe that's too confusing).

Okay @shoyer so the way I've gone about this deprecation problem is to essentially introduce manual_combine whilst reverting auto_combine. That means that auto_combine isn't a breaking change yet, but it's inputs are checked and if they will break in future it throws a warning saying "please use manual_combine instead because the behaviour of auto_combine will soon change".

This way we don't have two publicly-accessible versions of auto_combine present simultaneously, and if users heed the warnings and switch to using manual_combine then their code won't break when we later update auto_combine from the old to the new behaviour.

This approach means there currently isn't a way to opt-in to the new auto_combine behaviour early, but as you can opt-in to future-compatible behaviour by using manual_combine instead, then I think that's fine?

Okay so this is mostly done - I've completely rearranged the tests and they pass on my unmerged branch, there are just a few TODOs left.

~~However, when I tried to merge master in then everything failed again due to #2648, I don't understand what ReprObject does?~~ (Fixed with bfch4e3)

Some other questions:

1) We didn't decide whether the ability to infer concatenation dimensions (i.e. when concat_dim='_CONCAT_DIM_DEFAULT') should be retained or not. I'm not sure we need it at all any more - you're either using auto_combine, which will concatenate dimension coordinates and leave other dimensions alone, or you're using manual_combine, in which you're supposed to explicitly state all the dimensions you wish to concatenate along anyway. I think this is also related to a bug I don't properly understand (marked with an xfail in test_manual_concat_too_many_dims_at_once).

2) Ordering the datasets along one dimension according to the values in their dimension coordinates (done in _infer_order_1d) turned out to be a bit more complicated than I thought. Is the method using a Pandas.Series okay? Do we want to use natural sorting on strings? How should I deal with datetime objects? I also think it's probably easier to check that the coords can actually be arranged to be monotonic after all the concatenation has been done, not before.

3) Some input which previously wouldn't be valid now is (see the TODO in line 538 of test_combine.py), is that the desired behaviour in that case?