Skip to content

feat: add recursive flag to Content.to_packed#2933

Merged
jpivarski merged 3 commits into
mainfrom
agoose77/feat-to-packed-non-recursive
Jan 12, 2024
Merged

feat: add recursive flag to Content.to_packed#2933
jpivarski merged 3 commits into
mainfrom
agoose77/feat-to-packed-non-recursive

Conversation

@agoose77

@agoose77 agoose77 commented Jan 11, 2024

Copy link
Copy Markdown
Collaborator

This should fix #2916, which was caused by eager recursive packing of the array layout before unflattening.

This was first discussed in #910 (comment), where we understood that not packing the layout can lead to invalid unflatten operations.

This PR fixes #2916 by introducing a recursive: bool = True argument to Content.to_packed(). This allows us to perform a single-layout packing, which ensures that nodes below the current node are left untouched, reducing the numbers of buffers required at runtime.

I'm not 100% sure if this is the right API to go with. I think talking about packing non-recursively makes sense, rather than as a shoehorned bugfix. But, @jpivarski if you think otherwise let me know!

@codecov

codecov Bot commented Jan 11, 2024

Copy link
Copy Markdown

Codecov Report

Attention: 8 lines in your changes are missing coverage. Please review.

Comparison is base (6a0b318) 81.95% compared to head (bdb8933) 81.93%.

Additional details and impacted files
Files Coverage Δ
src/awkward/contents/bitmaskedarray.py 70.43% <100.00%> (-0.39%) ⬇️
src/awkward/contents/bytemaskedarray.py 89.65% <100.00%> (-0.11%) ⬇️
src/awkward/contents/content.py 74.81% <100.00%> (ø)
src/awkward/contents/emptyarray.py 75.12% <100.00%> (ø)
src/awkward/contents/indexedarray.py 81.03% <100.00%> (+0.08%) ⬆️
src/awkward/contents/indexedoptionarray.py 88.21% <100.00%> (+0.05%) ⬆️
src/awkward/contents/listarray.py 91.78% <100.00%> (ø)
src/awkward/contents/listoffsetarray.py 82.86% <100.00%> (ø)
src/awkward/contents/numpyarray.py 91.18% <100.00%> (ø)
src/awkward/contents/recordarray.py 85.19% <100.00%> (ø)
... and 5 more

@agoose77 agoose77 changed the title feat: add recusrive flag to Content.to_packed feat: add recursive flag to Content.to_packed Jan 11, 2024
@agoose77
agoose77 marked this pull request as ready for review January 11, 2024 14:42
@lgray

lgray commented Jan 11, 2024

Copy link
Copy Markdown
Collaborator

@cmoore24-24 Please check this out in your full analysis and see if it fixes things correctly in-situ. If the problem remains we'll have to understand if it's still the same issue and fix it in this PR or if it's a new issue altogether. (use awkward@main with this merged in)

@jpivarski jpivarski left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Adding a recursive: bool = True argument is definitely a good API.

We're just waiting for @cmoore24-24 to test it, and then I'd like to merge it to make a new awkward release. @agoose77, is there anything else you want to add before merging?

@jpivarski

Copy link
Copy Markdown
Member

From @lgray: let's merge this now and make a new release. If @cmoore24-24 encounters any problems, they'll be filed as new issues.

@jpivarski
jpivarski merged commit eb004ce into main Jan 12, 2024
@jpivarski
jpivarski deleted the agoose77/feat-to-packed-non-recursive branch January 12, 2024 16:07
@cmoore24-24

Copy link
Copy Markdown

Sorry for the delay and glad that this was merged; it has been entirely fixed in my full analysis. Thank you all for the help!

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.

flatten / unflatten operation touches whole records

4 participants