issue_comments

Pushing to a merged PR definitely doesn't work :). You should start a new branch after syncing master.

@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?

I started thinking about names for these functions again....

I don't love the non-descriptive nature of combine_auto and combine_manual. All "auto" and "manual" tells me is that one is more automatic than the other, without saying how it's more automatic. This is probably even more confusing because we used "auto" previously to mean some hybrid behavior between what we now call "manual" and "auto".

What about calling these combine_by_coords and combine_nested instead? On open_mfdataset, these would be combine='by_coords' and combine='nested'.

:tada: Thanks for your help!

Thanks @TomNicholas !

OK, in it goes! Thanks @TomNicholas for your perseverance here

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!

I left a couple of minor comments, but I think this is pretty much ready to go! We should merge this and let users give it a try :).

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?

Thanks @TomNicholas. You can use @pytest.mark.filterwarnings to filter specific warnings. I think that's what the decorator is called anyway. There are places where it's used in the testing code so you have plenty of examples to copy from.

@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.

Yes I saw that but missed https://github.com/pydata/xarray/blob/513764fa4094acda67aec5fea8397bc5726afb90/xarray/core/combine.py#L307

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?

Yes, I think you can add the examples from the docstrings to https://xarray.pydata.org/en/stable/combining.html since that's where users will look for such functionality.

@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?

I haven't reviewed the PR but noticed that there's no documentation to go along with it. Is that intentional?

@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.

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).

I think this is a good idea. We can deprecate the old auto_combine but keep it around for now to enable a smooth transition.

+1 for switching to combine_auto, combine_manual but not having both auto_combine and combine_auto.

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).

Do we want to stick with the names manual_combine and auto_combine?

Some other possibilities:

• Instead of manual_combine: concat_nd, concat_nested, multi_concat
• Instead of auto_combine:assemble, reassemble, combine_sorted, concat_sorted, unify

I guess auto/manual works well with the values of the keyword argument, and makes it clear that these are a different class of operation than the primitive concat and merge. Though if we really want to emphasize the similarity (especially to help auto-complete), we might consider even switching the order of the words, e.g., combine_auto and combine_manual.

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?