Replace SortedKeysDict with dict

[max-sixty]

• Tests added
• Passes isort . && black . && mypy . && flake8
• User visible changes (including notable bug fixes) are documented in whats-new.rst

Inspired by @mathause research in #4571 (comment)

Note that one test is removed: https://github.com/pydata/xarray/compare/master...max-sixty:sorted-keys?expand=1#diff-2db7f6624707083db4aaab1b62eb11352200ec9d3ac1055de84877912226d7b5L560

While I'm keen on simplifying the data structures, this may have some unforeseen consequences, and at least deserves some thought re how we handle differently ordered dims.

Currently this PR retains the sorting in reprs.

[max-sixty]

As discussed in dev meeting, this makes sense to pursue. We should try and remove the repr sorting if we can manage the test breaks.

[pep8speaks]

Hello @max-sixty! 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 2021-05-16 03:24:39 UTC

[max-sixty]

Is the existing behavior here a bug?

In [16]: x1.dimsOut[16]: Frozen({'y': 2, 'x': 3})
In [17]: x3.dimsOut[17]: Frozen({'y': 2, 'x': 3})
In [18]: xr.combine_by_coords([x3, x1], join="override")
Out[18]:
<xarray.Dataset>Dimensions: (y: 4, x: 3)
Coordinates:
*x (x) int64102030*y (y) int640123Datavariables:
temperature (y, x) float6410.9814.312.0610.9 ... 18.8910.448.293precipitation (y, x) float640.43760.89180.9637 ... 0.56840.018790.6176

and override is documented as:

"override": if indexes are of same size, rewrite indexes to be
those of the first object with that dimension. Indexes for the same
dimension must have the same size in all objects.

So should the result y dim have length 4?

This is changing behavior because the order of dims is changing.

[mathause]

Checking the repr of x1 and x2 reveals that both x and y are valid concat dimensions (given join="override") and it just chooses the first one (arbitrarily). I think this PR did not break it but just revealed a problem (also #4824). This should be fixed in another PR.

[max-sixty]

Thanks for that reference. I agree.

[TomNicholas]

Is this a dfferent bug from #4824?

[max-sixty]

@pydata/xarray do we have a preference on how we show constructors? I find the dict representation much easier to read, especially where we're passing tuples of lists etc.

I updated this with vim, could probably do the rest of them with some regex if people agree (different PR)

[keewis]

I don't really have a preference, but keep in mind that with the dict constructor you're restricted to python identifiers while the dictionary literal takes any hashable (not sure how much of a restricting that would be, though).

[max-sixty]

Yes, agree. As you suggest though — these examples almost exclusively use strings.

[max-sixty]

This ended up being more work than expected, but I think is ready for review. There's one issue that may be an existing bug (but may be my misunderstanding).

[mathause]

No strong opinion on the dicts. Looks good overall.

I think combine_by_coords is brittle in case of ambiguous concat dims as in the given example. Before this change the concat dim depended on the name of the dimensions (i.e. their alphabetical ordering), now it depends on the insertion order. I am not sure what's worse. We should absolutely fix that (also #4824).

[mathause]

Checking the repr of x1 and x2 reveals that both x and y are valid concat dimensions (given join="override") and it just chooses the first one (arbitrarily). I think this PR did not break it but just revealed a problem (also #4824). This should be fixed in another PR.

[max-sixty]

Ready to go!

[TomNicholas]

I think combine_by_coords is brittle in case of ambiguous concat dims as in the given example

@mathause if this is a new bug then can you make an issue for it please?

[max-sixty]

I think combine_by_coords is brittle in case of ambiguous concat dims as in the given example

@mathause if this is a new bug then can you make an issue for it please?

Isn't this #4824?

[keewis]

this is a good way to work around not being able to sort keys of different types.

I worry about the dimension sizes which are part of the first line of the Dataset repr, though: we've had lots of questions about this because people thought that it would mean the same as in a DataArray's repr (the order there has a meaning). Arguably, that's because the difference between them is not big enough. This seems to have become less frequent since the introduction of the HTML reprs, so maybe it is also less important?

[max-sixty]

I worry about the dimension sizes which are part of the first line of the Dataset repr, though: we've had lots of questions about this because people thought that it would mean the same as in a DataArray's repr (the order there has a meaning). Arguably, that's because the difference between them is not big enough. This seems to have become less frequent since the introduction of the HTML reprs, so maybe it is also less important?

I agree the current situation isn't ideal (though I'm not sure there's a better one) but also that this doesn't make it much worse — the existing approach doesn't represent an useful ordering, and nor does this. And if we choose to make it mean something in the future, then this is a necessary step towards that...

[shoyer]

This looks great to me, thanks!

I wonder if we should hold off on merging until after the 0.18.1 release -- or maybe just move to a date based versioning system / release cycle like dask.