Skip to content

Maximize coverage #542

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Mar 8, 2020
Merged

Maximize coverage #542

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
317ff89
Drop Python 2 only code
jakirkham Mar 3, 2020
159ccc3
Smooth over NumPy `unravel_index` kwargs
jakirkham Mar 3, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 1 addition & 5 deletions zarr/indexing.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -691,11 +691,7 @@ def __init__(self, selection, array):
self.chunk_rixs = np.nonzero(self.chunk_nitems)[0]

# unravel chunk indices
if tuple(map(int, np.__version__.split('.')[:2])) < (1, 16):
self.chunk_mixs = np.unravel_index(self.chunk_rixs, dims=array._cdata_shape)
else:
# deal with change dims->shape in arguments as of numpy 1.16
self.chunk_mixs = np.unravel_index(self.chunk_rixs, shape=array._cdata_shape)
self.chunk_mixs = np.unravel_index(self.chunk_rixs, array._cdata_shape)
Copy link
Member Author

Choose a reason for hiding this comment

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

Do you know if the keyword argument was needed for some reason @alimanfoo? Noticed this change came from here. Hence the question 🙂

Copy link
Member

Choose a reason for hiding this comment

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

In general, I think using kwargs is better when it's defined as a kwarg rather than relying on the order of positional args. The previous block looked like it was here to deal with numpy <1.16 and numpy >1.16 but in our setup.py we specify numpy>=1.17, so I think it's safe to drop handling old numpy versions and use shape= but not fussed too much either way.

Copy link
Member

Choose a reason for hiding this comment

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

IIUC our setup.py has NumPy 1.7 as the minimum supported version

zarr-python/setup.py

Lines 12 to 17 in 3ae9439

dependencies = [
'asciitree',
'numpy>=1.7',
'fasteners',
'numcodecs>=0.6.4',
]

Copy link
Member

Choose a reason for hiding this comment

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

Yep. :)

Copy link
Member

Choose a reason for hiding this comment

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

So I guess we do need to consider older versions of NumPy where the signature keywords may have been different

Copy link
Member

Choose a reason for hiding this comment

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

Ah, I kept reading 1.7 as 1.17... 🤦‍♂

Copy link
Member Author

Choose a reason for hiding this comment

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

Seems to be largely unchanged over time. 🙂 Here are the docs from NumPy 1.7.

Copy link
Member

Choose a reason for hiding this comment

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

Cool, cool. Happy for this to be merged on my side :)


def __iter__(self):

Expand Down
2 changes: 0 additions & 2 deletions zarr/tests/test_meta.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,8 +13,6 @@


def assert_json_equal(expect, actual):
if isinstance(expect, bytes): # pragma: py3 no cover
expect = str(expect, 'ascii')
if isinstance(actual, bytes):
actual = str(actual, 'ascii')
ej = json.loads(expect)
Expand Down
9 changes: 0 additions & 9 deletions zarr/util.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -510,15 +510,6 @@ def is_valid_python_name(name):
return name.isidentifier() and not iskeyword(name)


def class_dir(klass): # pragma: py3 no cover
d = dict()
d.update(klass.__dict__)
bases = klass.__bases__
for base in bases:
d.update(class_dir(base))
return d


class NoLock(object):
"""A lock that doesn't lock."""

Expand Down