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

Fix: _pack_layout should also pack projected index arrays #1195

Merged
merged 1 commit into from
Dec 28, 2021

Conversation

agoose77
Copy link
Collaborator

@agoose77 agoose77 commented Dec 28, 2021

When _pack_layout is passed an indexed array, it returns layout.project(). However, _pack_layout is supposed to return a layout whereby only its children may need packing. In this case, by returning layout.project(), we are not guaranteed to satisfy this expectation.

This PR fix the issue by ensuring that we pack the result of project.

When _pack_layout is passed an indexed array, it returns `layout.project()`. The API of `_pack_layout` is such that it is expected to return a packed layout. In this case, the result of `layout.project()` may well not be packed, and would require an additional pass.
@codecov
Copy link

codecov bot commented Dec 28, 2021

Codecov Report

Merging #1195 (02c64e3) into main (9955401) will increase coverage by 1.53%.
The diff coverage is 83.23%.

Impacted Files Coverage Δ
src/awkward/_v2/_connect/pyarrow.py 83.33% <0.00%> (-0.48%) ⬇️
src/awkward/_v2/_reducers.py 96.48% <ø> (ø)
src/awkward/_v2/forms/recordform.py 66.46% <0.00%> (ø)
src/awkward/_v2/operations/convert/ak_to_numpy.py 100.00% <ø> (ø)
...c/awkward/_v2/operations/structure/ak_unflatten.py 80.00% <ø> (ø)
src/awkward/_v2/types/arraytype.py 80.76% <0.00%> (-0.72%) ⬇️
src/awkward/_v2/contents/unmaskedarray.py 56.27% <51.72%> (+0.82%) ⬆️
src/awkward/_v2/contents/indexedarray.py 59.75% <63.23%> (+0.44%) ⬆️
src/awkward/_v2/_typetracer.py 69.19% <63.58%> (-0.36%) ⬇️
src/awkward/_v2/contents/emptyarray.py 69.54% <74.19%> (+0.26%) ⬆️
... and 31 more

@jpivarski
Copy link
Member

ak.packed has already been translated to v2; is this issue present in the translated function? We don't want the bug to regress when we update. Thanks!

@agoose77
Copy link
Collaborator Author

agoose77 commented Dec 28, 2021

This should already be fixed in v2 after looking at the source: we immediately call packed() on the project()ed result

Copy link
Member

@jpivarski jpivarski left a comment

Choose a reason for hiding this comment

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

Great! Then if you think it's done, you can go ahead and "squash and merge" it.

(I just checked: you should have access to do so.)

@agoose77 agoose77 merged commit 1751ffc into main Dec 28, 2021
@agoose77 agoose77 deleted the fix-pack-layout-index-array branch December 28, 2021 19:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants