Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Datasets more robust to non-string keys #2174

Merged
merged 16 commits into from
May 27, 2018
1 change: 1 addition & 0 deletions setup.cfg
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ testpaths=xarray/tests
[flake8]
max-line-length=79
ignore=
W503
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is line break before operator, which has been changed in PEP8

exclude=
doc/

Expand Down
12 changes: 11 additions & 1 deletion xarray/core/dataarray.py
Original file line number Diff line number Diff line change
Expand Up @@ -466,7 +466,17 @@ def _getitem_coord(self, key):
return self._replace_maybe_drop_dims(var, name=key)

def __getitem__(self, key):
if isinstance(key, basestring):
try:
is_coord_key = any([
isinstance(key, basestring),
key in set(self.dims).union(self.coords)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would hesitate to do this fix -- it means array[2] could pull out a coordinate if 2 is the name of a dimension. More generally, DataArray.__getitem__ is only meant as a convenient short-cut for getting coordinates, which we might even want to deprecate entirely. DataArray.coords.__getitem__ is the reliable way to get a coordinate out.

Instead, I think the better option would be to avoid making use of ** unpacking below, in the expression self.isel(**self._item_key_to_dict(key)). This would be a little more involved, but I think the right fix would be to make most of these functions accept a positional dictionaries instead of **kwargs, as suggested in #1231.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, completely agree, thanks

])
except TypeError:
# not hashable, but testing with collections.Hashable is not
# complete, since# tuples with slices inside will suggest
# they're hashable
is_coord_key = False
if is_coord_key:
return self._getitem_coord(key)
else:
# xarray-style array indexing
Expand Down
4 changes: 2 additions & 2 deletions xarray/tests/test_dataarray.py
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,8 @@
from xarray.core.pycompat import OrderedDict, iteritems
from xarray.tests import (
ReturnItem, TestCase, assert_allclose, assert_array_equal, assert_equal,
assert_identical, raises_regex, requires_bottleneck, requires_dask,
requires_scipy, source_ndarray, unittest, requires_cftime)
assert_identical, raises_regex, requires_bottleneck, requires_cftime,
requires_dask, requires_scipy, source_ndarray, unittest)


class TestDataArray(TestCase):
Expand Down
6 changes: 6 additions & 0 deletions xarray/tests/test_dataset.py
Original file line number Diff line number Diff line change
Expand Up @@ -4181,6 +4181,12 @@ def test_dir_non_string(data_set):
result = dir(data_set)
assert not (5 in result)

# GH2172
sample_data = np.random.uniform(size=[2, 2000, 10000])
x = xr.Dataset({"sample_data": (sample_data.shape, sample_data)})
x2 = x["sample_data"]
dir(x2)


def test_dir_unicode(data_set):
data_set[u'unicode'] = 'uni'
Expand Down