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

RecordArray loses its field in a slice that doesn't select fields (v2) #1593

Closed
jpivarski opened this issue Aug 15, 2022 · 0 comments · Fixed by #1597
Closed

RecordArray loses its field in a slice that doesn't select fields (v2) #1593

jpivarski opened this issue Aug 15, 2022 · 0 comments · Fixed by #1597
Assignees
Labels
bug The problem described is something that must be fixed

Comments

@jpivarski
Copy link
Member

Version of Awkward Array

HEAD

Description and code to reproduce

Here's a good slice:

>>> ak._v2.Array([[[1, 2, 3], [4, 5, 6]]])[:, []]
<Array [[[], []]] type='1 * var * var * int64'>

There's no record, no field being picked out, and it says, "all of the first dimension, none of the second." The result is correct.

Here's another good slice:

>>> ak._v2.Array([{"x": [[1, 2, 3], [4, 5, 6]]}])[:, [0]]
<Array [{x: [[1, 2, 3]]}] type='1 * {x: 1 * var * int64}'>

There's a record array, but a field is not picked out, and it says, "all of the first dimension, one item from the second." The result is correct.

But here's a bad slice:

>>> ak._v2.Array([{"x": [[1, 2, 3], [4, 5, 6]]}])[:, []]
<Array [{}] type='1 * {}'>

There's a record array, but a field is not picked out, and it says, "all of the first dimension, none of the second." The result is a record array with no fields: that's wrong—there should still be an x field, even though it will be empty.

Somewhere, an ak._v2.contents.RecordArray is not being populated correctly.

@jpivarski jpivarski added the bug The problem described is something that must be fixed label Aug 15, 2022
@agoose77 agoose77 self-assigned this Aug 25, 2022
agoose77 added a commit that referenced this issue Aug 26, 2022
* Fix: tentative fix for empty slice lists

* Test: validate fix

* Update tests/v2/test_1593-empty-slice-list-record.py

Co-authored-by: ioanaif <[email protected]>

* Fix: convert `[]` into `slice(0, 0)`

This bypasses the `slice_fields` mechanism, which is really orthogonal to the content length `_getitem` implementations.

* Refactor: check explicitly for iterables with lengths

* Test: fix to match v1

* Fix: normalise [] to empty array

* Fix: return `NumpyLike.array` instead of index

* Fix: support empty Index64

* Docs: add small docstring elucidating getitem_broadcast

* Fix: update `getitem_next_array_wrap` to accept `outer_length` argument

* Fix: remove list special-case

* Fix: pass list length to `getitem_next_array_wrap`

* feat: raise error if unsupported indexing used

This prevents the second-case of advanced indexing referred to in https://numpy.org/doc/stable/user/basics.indexing.html#basics-indexing:~:text=In%20the%20first,just%20like%20slicing from being used

* test: fix use of v1 record

* fix: respect zero-length of regular array

* test: respect new error during indexing

* test: fix expected output of slice

* test: fix expected output of slice

* test: fix expected output of slice

* fix: return view in `TypeTracerArray.view`

* feat: add `TypeTracerArray.astype`

* feat: raise `ValueError` for mixed indices

This changes #1305

* refactor: move NumPy index validation into broadcasting

This also renames various utility functions to make the concerns more apparent.

* fix: actually raise exception

* refactor: remove unneeded length check

Co-authored-by: ioanaif <[email protected]>
Saransh-cpp pushed a commit to Saransh-cpp/awkward that referenced this issue Aug 29, 2022
…#1597)

* Fix: tentative fix for empty slice lists

* Test: validate fix

* Update tests/v2/test_1593-empty-slice-list-record.py

Co-authored-by: ioanaif <[email protected]>

* Fix: convert `[]` into `slice(0, 0)`

This bypasses the `slice_fields` mechanism, which is really orthogonal to the content length `_getitem` implementations.

* Refactor: check explicitly for iterables with lengths

* Test: fix to match v1

* Fix: normalise [] to empty array

* Fix: return `NumpyLike.array` instead of index

* Fix: support empty Index64

* Docs: add small docstring elucidating getitem_broadcast

* Fix: update `getitem_next_array_wrap` to accept `outer_length` argument

* Fix: remove list special-case

* Fix: pass list length to `getitem_next_array_wrap`

* feat: raise error if unsupported indexing used

This prevents the second-case of advanced indexing referred to in https://numpy.org/doc/stable/user/basics.indexing.html#basics-indexing:~:text=In%20the%20first,just%20like%20slicing from being used

* test: fix use of v1 record

* fix: respect zero-length of regular array

* test: respect new error during indexing

* test: fix expected output of slice

* test: fix expected output of slice

* test: fix expected output of slice

* fix: return view in `TypeTracerArray.view`

* feat: add `TypeTracerArray.astype`

* feat: raise `ValueError` for mixed indices

This changes scikit-hep#1305

* refactor: move NumPy index validation into broadcasting

This also renames various utility functions to make the concerns more apparent.

* fix: actually raise exception

* refactor: remove unneeded length check

Co-authored-by: ioanaif <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug The problem described is something that must be fixed
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants