feat: add recursive flag to Content.to_packed
#2933
Conversation
Codecov ReportAttention:
Additional details and impacted files
|
agoose77
changed the title
recusrive flag to Content.to_packedrecursive flag to Content.to_packed
|
@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) |
There was a problem hiding this comment.
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?
|
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. |
|
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! |
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 = Trueargument toContent.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!