Rework DataArray internals

[shoyer]

Fixes #367
Fixes #634
Fixes #649

The internal data model used by DataArray has been rewritten to fix several outstanding issues (#367, #634 and this stackoverflow report). Namely, if a DataArray has the same name as one of its coordinates, the array and the coordinate no longer share the same data.

This means that creating a DataArray with the same name as one of its dimensions no longer automatically uses that array to label the corresponding coordinate. You will now need to provide coordinate labels explicitly. Here's the old behavior:

In [2]: xray.DataArray([4, 5, 6], dims='x', name='x')
Out[2]:
<xray.DataArray 'x' (x: 3)>
array([4, 5, 6])
Coordinates:
  * x        (x) int64 4 5 6

and the new behavior (compare the values of the x coordinate):

In [2]: xray.DataArray([4, 5, 6], dims='x', name='x')
Out[2]:
<xray.DataArray 'x' (x: 3)>
array([4, 5, 6])
Coordinates:
  * x        (x) int64 0 1 2

It's also no longer possible to convert a DataArray to a Dataset with DataArray.to_dataset if it is unnamed. This will now raise ValueError. If the array is unnamed, you need to supply the name argument.

[jhamman]

@shoyer - I read this mainly trying to get a better idea of the internal DataArray data model. The code itself looks great. My main two comments on the refactor are:

1. The use of the _ThisArray singleton seems a bit clunky to me. This is my first time interacting with this approach so maybe it is the best way, but I don't know much about it.
2. This all seems to be complicated by the DataArrays's use of the Dataset internally to store coordinates. Not for this PR, but I wonder if there is a way to clear up the lines between the two objects.

All in all, impressive work.

[shoyer]

Indeed, I wonder if it would make sense to decouple DataArray from Dataset by storing the state on two (protected) attributes:

• variable: the variable for this array
• coords: an ordered dict of coordinates

The main downside is that we add a bit more redundant code (e.g., to loop over all variables in .sel). But on the plus side our data model is much more similar to the public API, which would probably make things easier to understand, especially for new contributors -- they don't have to learn Dataset to learn DataArray.

[max-sixty]

Indeed, I wonder if it would make sense to decouple DataArray from Dataset by storing the state on two (protected) attributes:

As a newbie, 👍. I took some time to figure out why a DataArray contained a Dataset.

The main downside is that we add a bit more redundant code (e.g., to loop over all variables in .sel).

Low confidence, but you could have a common ancestor (XRayContainer, etc) which contained the similar code or a ._stuff attribute that's iterated over

[max-sixty]

Very minor, but name = name or self.name might be clearer / more pythonic than the repeated if name is None:

[jhamman]

@shoyer - do you have a feel for how difficult it would be to go the variable/coords route?

[shoyer]

@shoyer - do you have a feel for how difficult it would be to go the variable/coords route?

Hmm. Might not be so bad now that I've already gone through the trouble of thinking what these new tests should look like. I'll give it a shot tonight and see how it goes...

[shoyer]

I realize now that changing the internal representation for DataArray doesn't mean we need to rewrite how every routine works. We can still convert dataarrays to a dataset when convenient -- it just means we'll need to use a method to do so instead of modifying ._dataset. For example, we currently have:

    def copy(self, deep=True):
        ds = self._dataset.copy(deep=deep)
        return self._with_replaced_dataset(ds)

and instead we could simply write:

    def copy(self, deep=True):
        ds = self._to_temp_dataset().copy(deep=deep)
        return self._new_from_temp_dataset(ds)

However, going forward it will give us more flexibility for how to write DataArray methods. For example, it might actually be clearer to write:

    def copy(self, deep=True):
        variable = self.variable.copy(deep=deep)
        coords = OrderedDict((k, v.copy(deep=deep))
                             for k, v in self._coords.items())
        return type(self)(variable, coords, name=name, fastpath=True)

[shoyer]

OK, latest commit changes DataArray's internals to rely on _variable and _coords instead of _dataset. There's still a bit more _to_temp_dataset() than ideal, but hopefully the data model makes more sense now...

[shoyer]

This is ready for review if anyone wants to take another look.

[jhamman]

what's going on here?

[shoyer]

Good catch -- needed explanation. Let me know if the comments I added help.

[jhamman]

@shoyer - I don't have any more inline comments. There is one failing test and there are merge conflicts, once those are addressed, I'll take one more brief look.

[shoyer]

Rebased and tests are passing.

[jhamman]

lgtm, go ahead and merge.

[max-sixty]

👏
I'll try and get my PR in this weekend for the pandas wrapping