issue_comments

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

lgtm, go ahead and merge.

Rebased and tests are passing.

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

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

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

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:

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

and instead we could simply write:

python 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:

python 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 - 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 - do you have a feel for how difficult it would be to go the variable/coords route?

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

As a newbie, :+1:. 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

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.

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