WIP: don't create indexes on multidimensional dimensions

[rabernat]

• Closes Let's list all the netCDF files that xarray can't open #2368, Closes Problem opening unstructured grid ocean forecasts with 4D vertical coordinates #2233
• Tests added (for all bug fixes or enhancements)
• Tests passed (for all non-documentation changes)
• Fully documented, including whats-new.rst for all changes and api.rst for new API (remove if this change should not be visible to users, e.g., if it is an internal clean-up, or if this is part of a larger project that will be documented later)

This is just a start to the solution proposed in #2368. A surprisingly small number of tests broke in my local environment.

[rabernat]

I tried moving this import to the top of formatting.py but, for reasons I can't understand, I got this error

    from .variable import IndexVariable
E   ImportError: cannot import name 'IndexVariable

Why does it work in the function but not at the top?

[markelg]

I think there is a circular import going on here, as .variable does import .common which does import .formatting Perhaps adding a self.is_index attribute to the Variable class could be a way to avoid this import?

[shoyer]

Yes, we have a circular import here. We could potentially refactor to avoid but I think this is also an OK work-around.

[rabernat]

Seeking suggestions about what else needs to be tested here.

[shoyer]

I turned up a few lines that probably need to be fixed, just by grepping for in\ \w+\.dims:

xarray/xarray/backends/api.py

Lines 125 to 126 in 66a8f8d

	if name not in variable.dims:
	# no need to protect IndexVariable objects

xarray/xarray/core/coordinates.py

Lines 111 to 116 in 66a8f8d

	# don't include indexes in priority_vars, because we didn't align
	# first
	priority_vars = OrderedDict(
	kv for kv in self.variables.items() if kv[0] not in self.dims)
	variables = merge_coords_for_inplace_math(
	[self.variables, other.variables], priority_vars=priority_vars)

xarray/xarray/core/dataarray.py

Line 502 in 66a8f8d

return [self.coords, {d: self.coords[d] for d in self.dims},

It looks like this change broke align somehow -- possibly we have a bug where some indexes were not being created as IndexVariable objects?

[shoyer]

It would be good to add a tests for these variables in:

• The data_vars argument to the Dataset constructor (these objects should become data_vars, not coords)
• The DataArray constructor.
• Explicitly adding these variables:
  • With Dataset.__setitem__, Dataset.coords.__setitem__ and DataArray.coords.__setitem__
  • When there are no existing variables matching the dimension name, and when a 1D index variable matching the dimension name already exists (the existing index should be deleted/cleared)
• Converting these objects to pandas with to_dataframe, to_series and to_pandas
• Concatenating along an existing dimension whose name matches existing variables (e.g., concatenate along x when a variable x with dimensions ('x', 'y') exists)
• Concatenating along a new dimension whose name matches existing variables (e.g., concatenate along x when a variable x with dimensions ('y',) exists)
• More generally, test variables where the name matches a dimension but the variable does not include that dimension at all, e.g., a variable 'x' with dimensions ('y',) in a Dataset where x is a dimension.

[rabernat]

With 68f170c there are basically no failing tests in the original test suite. I will now work on adding more tests along the lines outlined by @shoyer.

[pep8speaks]

Hello @rabernat! Thanks for updating the PR.

• In the file xarray/core/formatting.py, following are the PEP8 issues :

Line 275:45: E225 missing whitespace around operator

• In the file xarray/core/indexes.py, following are the PEP8 issues :

Line 55:77: E225 missing whitespace around operator
Line 55:80: E501 line too long (80 > 79 characters)

• In the file xarray/tests/test_dataset.py, following are the PEP8 issues :

Line 68:1: E302 expected 2 blank lines, found 1
Line 251:9: E265 block comment should start with '# '

[rabernat]

I had some spare time and started working on this again. I'm scared of how much internal refactoring it requires. For example, this function

xarray/xarray/core/dataset.py

Lines 92 to 113 in 385b36c

	def calculate_dimensions(variables):
	"""Calculate the dimensions corresponding to a set of variables.
	Returns dictionary mapping from dimension names to sizes. Raises ValueError
	if any of the dimension sizes conflict.
	"""
	dims = OrderedDict()
	last_used = {}
	scalar_vars = set(k for k, v in iteritems(variables) if not v.dims)
	for k, var in iteritems(variables):
	for dim, size in zip(var.dims, var.shape):
	if dim in scalar_vars:
	raise ValueError('dimension %r already exists as a scalar '
	'variable' % dim)
	if dim not in dims:
	dims[dim] = size
	last_used[dim] = k
	elif dims[dim] != size:
	raise ValueError('conflicting sizes for dimension %r: '
	'length %s on %r and length %s on %r' %
	(dim, size, k, dims[dim], last_used[dim]))
	return dims

just doesn't make sense any more if dimensions are not guaranteed to be one-dimensional.

I've never touched this part of the code base before. I have no idea how many places there are that make such an assumption.

[shoyer]

I think this function mostly makes sense, but we would want to drop the stuff for scalar variables.

Unfortunately I don't know a good way to fix this stuff short of auditing a lot of code manually -- this is a hazard of data model changes. On the plus side, I've also started to do some of this for the explicit index refactor.

One thing that might turn up a few bugs is to try adding such a variable to the create_test_data() helper function which gets used in lots of places.