Skip to content

Enable slice by 0-dimensional np.array#9558

Open
fandreuz wants to merge 2 commits intodask:mainfrom
fandreuz:fix-3406
Open

Enable slice by 0-dimensional np.array#9558
fandreuz wants to merge 2 commits intodask:mainfrom
fandreuz:fix-3406

Conversation

@fandreuz
Copy link
Contributor

@fandreuz fandreuz commented Oct 8, 2022

@GPUtester
Copy link
Collaborator

Can one of the admins verify this patch?

Admins can comment ok to test to allow this one PR to run or add to allowlist to allow all future PRs from the same author to run.

@github-actions github-actions bot added the array label Oct 8, 2022
@fandreuz fandreuz changed the title enable slice by 0-dimensional np.array Enable slice by 0-dimensional np.array Oct 8, 2022
@fandreuz fandreuz changed the title Enable slice by 0-dimensional np.array Enable slice by 0-dimensional np.array Oct 8, 2022
@fandreuz
Copy link
Contributor Author

fandreuz commented Oct 8, 2022

As far as I can tell test failures are not related with my changes

@ncclementi
Copy link
Member

ok to test

Copy link
Member

@jrbourbeau jrbourbeau left a comment

Choose a reason for hiding this comment

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

Thanks @fAndreuzzi!

Comment on lines +418 to +467
def test_slicing_row_with_0d_numpy_arrays():
a, bd1 = slice_array(
"y",
"x",
((3, 3, 3, 2), (3, 3, 3, 1)),
(np.array(0), slice(None, None, None)),
itemsize=8,
)

i = [True] + [False] * 10
index = (i, slice(None, None, None))
index = normalize_index(index, (11, 10))
b, bd2 = slice_array("y", "x", ((3, 3, 3, 2), (3, 3, 3, 1)), index, itemsize=8)

# bd1=((3, 3, 3, 1),)
# bd2=((1,), (3, 3, 3, 1))
assert bd1[0] == bd2[1]
for key_b, value in b.items():
if key_b[0] == "x":
key_a = key_b
elif key_b[0] == "y":
key_a = key_b[::2]
np.testing.assert_equal(a[key_a], value)


def test_slicing_col_with_0d_numpy_arrays():
a, bd1 = slice_array(
"y",
"x",
((3, 3, 3, 1), (3, 3, 3, 2)),
(slice(None, None, None), np.array(0)),
itemsize=8,
)

i = [True] + [False] * 10
index = (slice(None, None, None), i)
index = normalize_index(index, (10, 11))
b, bd2 = slice_array("y", "x", ((3, 3, 3, 1), (3, 3, 3, 2)), index, itemsize=8)

# bd1=((3, 3, 3, 1),)
# bd2=((3, 3, 3, 1), (1,))
assert bd1[0] == bd2[0]
for key_b, value in b.items():
if key_b[0] == "x":
key_a = key_b
elif key_b[0] == "y":
key_a = key_b[:2]
np.testing.assert_equal(a[key_a], value)


Copy link
Member

Choose a reason for hiding this comment

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

I'm curious why these tests are written this way. I think the intent of this test would be more clear if we used simple user-facing APIs. For example, using the problematic snippet provided in the original issue, something like

def test_slicing_with_0d_numpy_arrays():
    x = da.arange(10, chunks=2)
    x_np = np.arange(10)

    actual = x[np.array(0)]
    expected = x_np[np.array(0)]
    assert_eq(actual, expected)

Should, I think, test what we're after with the changes in this PR. @fAndreuzzi thoughts?

Copy link
Contributor Author

@fandreuz fandreuz Oct 24, 2022

Choose a reason for hiding this comment

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

To be honest I've simply tried to mimic some of the tests I found in test_slicing.py, I think they test that chunks are not cut in weird ways after slicing. But I know very little of the codebase, so I can for sure change the tests to what you suggested @jrbourbeau

@github-actions github-actions bot added the needs attention It's been a while since this was pushed on. Needs attention from the owner or a maintainer. label May 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

array needs attention It's been a while since this was pushed on. Needs attention from the owner or a maintainer.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Slice by 0-dimensional np.array

4 participants