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

Revert "Fix is_total_slice for size-1 dimensions (#1800)" #1882

Closed
wants to merge 2 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
3 changes: 3 additions & 0 deletions docs/release.rst
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,9 @@ Unreleased

Enhancements
~~~~~~~~~~~~
* Revert "Performance improvement for reading and writing chunks if any of the dimensions is size 1."
By :user:`Deepak Cherian <dcherian>`. :issue:`1874`.


Docs
~~~~
Expand Down
44 changes: 44 additions & 0 deletions zarr/tests/test_core.py
Original file line number Diff line number Diff line change
Expand Up @@ -3157,3 +3157,47 @@ def test_issue_1279(tmpdir):

written_data = ds_reopened[:]
assert_array_equal(data, written_data)


def test_scalar_indexing():
# regression test for #1874
store = zarr.KVStore({})

store["a"] = zarr.create((3,), chunks=(1,), store=store)
store["a"][:] = [1, 2, 3]

assert store["a"][1] == np.array(2.0)
assert store["a"][(1,)] == np.array(2.0)

store["a"][0] = [-1]
assert store["a"][0] == np.array(-1)

store["a"][0] = -2
assert store["a"][0] == np.array(-2)

store["a"][0] = (-3,)
assert store["a"][0] == np.array(-3)


def test_object_array_indexing():
# regression test for #1874
from numcodecs import MsgPack

root = zarr.group()
arr = root.create_dataset(
name="my_dataset",
shape=0,
dtype=object,
object_codec=MsgPack(),
)
new_items = [
["A", 1],
["B", 2, "hello"],
]
arr_add = np.empty(len(new_items), dtype=object)
arr_add[:] = new_items
arr.append(arr_add)

elem = ["C", 3]
arr[0] = elem
assert arr[0] == elem
9 changes: 0 additions & 9 deletions zarr/tests/test_util.py
Original file line number Diff line number Diff line change
Expand Up @@ -89,15 +89,6 @@ def test_is_total_slice():
assert not is_total_slice((slice(0, 50), slice(0, 50)), (100, 100))
assert not is_total_slice((slice(0, 100, 2), slice(0, 100)), (100, 100))

# size-1 dimension edge-case
# https://github.com/zarr-developers/zarr-python/issues/1730
assert is_total_slice((slice(0, 1),), (1,))
# this is an equivalent selection (without a slice)
assert is_total_slice((0,), (1,))
# same for multidimensional selection
assert is_total_slice((slice(0, 1), slice(0, 10)), (1, 10))
assert is_total_slice((0, slice(0, 10)), (1, 10))

with pytest.raises(TypeError):
is_total_slice("foo", (100,))

Expand Down
13 changes: 2 additions & 11 deletions zarr/util.py
Original file line number Diff line number Diff line change
Expand Up @@ -234,17 +234,8 @@ def is_total_slice(item, shape: Tuple[int]) -> bool:
if isinstance(item, tuple):
return all(
(
(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Alternatively I could comment this out and add a TODO

Copy link
Member

Choose a reason for hiding this comment

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

From my side, +1 for reverting. We can also revert the revert.

isinstance(it, slice)
and (
(it == slice(None))
or ((it.stop - it.start == sh) and (it.step in [1, None]))
)
)
# The only scalar edge case, indexing with int 0 along a size-1 dimension
# is identical to a total slice
# https://github.com/zarr-developers/zarr-python/issues/1730
or (isinstance(it, int) and it == 0 and sh == 1)
isinstance(it, slice)
and ((it == slice(None)) or ((it.stop - it.start == sh) and (it.step in [1, None])))
)
for it, sh in zip(item, shape)
)
Expand Down
Loading