Remove old auto combine

[TomNicholas]

• Finishes deprecation cycle started in API for N-dimensional combine #2616 (was supposed to have been done in 0.15)
• Passes isort -rc . && black . && mypy . && flake8
• Fully documented, including whats-new.rst for all changes and api.rst for new API

I've set combine='by_coords' as the default argument to open_mfdataset. Technically we could go for either, as the deprecation warning just told users to make it explicit from now on, but going for by_coords rather than nested means that:

• The concat_dim argument is not needed by default,
• The default behaviour of the function is the "magic" one - users have to opt-in to the more explicit behaviour.

[pep8speaks]

Hello @TomNicholas! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2020-06-24 14:38:55 UTC

[TomNicholas]

It would also be good to include any improvement to the relevant docs (e.g. #3830) in the same release.

[keewis]

To make the docs CI pass, remove auto_combine from api-hidden.rst and api.rst (but see below). We also seem to have a new group of failing cftime tests that probably deserve their own issue.

Shouldn't we change the FutureWarning to a DeprecationWarning before removing auto_combine? Both your entry to whats-new.rst and the message of the warning read that way... I'm not sure what the rules for deprecations are, though.

[max-sixty]

I think FutureWarning and DeprecationWarning are pretty similar — mostly focused on whether the behavior will change or the API is going away.
FutureDeprecationWarning PendingDeprecationWarning is generally lower visibility IIRC.

So maybe this should have been DeprecationWarning but no need to go through another cycle imo.

[dcherian]

There was a case I ran in to where the old auto_combine was needed and none of the newer options worked.

Let me see if I can dig it up.

[TomNicholas]

To make the docs CI pass

Thanks @keewis .

Shouldn't we change the FutureWarning to a DeprecationWarning before removing auto_combine?

I wasn't even aware that there were two types of warnings I could have used for this. I was taking "deprecating" to mean "removed" and "starting the deprecation cycle" to mean "start warning people it will soon be removed". The warning messages said "will no longer accept" and "will require" in "version 0.15" - that's probably authoritative enough isn't it?

There was a case I ran in to where the old auto_combine was needed and none of the newer options worked.

I think that was always a possible edge case - I would very much like to see it though @dcherian , if only so that we can put a "what to do if" section in the docs.

[keewis]

The warning messages said "will no longer accept" and "will require" in "version 0.15" - that's probably authoritative enough isn't it?

I'd think so, yes. Rereading what the warnings are intended for, FutureWarning is indeed the same as DeprecationWarning, the only difference is the intended target audience. I seem to have confused FutureWarning with PendingDeprecationWarning.

Then my only issue is with the whats-new.rst entry. I'd explicitly state that auto_combine was removed.

Edit: sorry, you also mentioned that. The way I understood "deprecating" was what you meant with "starting the deprecation cycle", so that's where the confusion came from.

[keewis]

thanks, @TomNicholas, looks good to me.

I opened #3928 for the upstream-dev failures.

[TomNicholas]

Just noticed there's a section of the API reference for Deprecated/Pending Deprecation, but auto_combine was never added to it...

[dcherian]

Thanks @TomNicholas