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

Enable slice by 0-dimensional np.array #9558

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

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

dask/array/slicing.py Outdated Show resolved Hide resolved
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!

dask/array/slicing.py Outdated Show resolved Hide resolved
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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Slice by 0-dimensional np.array
4 participants