Skip to content

feat: skip materialization of placeholders in to_packed#3637

Merged
ikrommyd merged 5 commits intoscikit-hep:mainfrom
ikrommyd:do-not-materialize-in-to-packed
Sep 5, 2025
Merged

feat: skip materialization of placeholders in to_packed#3637
ikrommyd merged 5 commits intoscikit-hep:mainfrom
ikrommyd:do-not-materialize-in-to-packed

Conversation

@ikrommyd
Copy link
Copy Markdown
Collaborator

@ikrommyd ikrommyd commented Sep 5, 2025 •
edited
Loading

This PRs avoids the materialization of placeholder arrays in to_packed to not cause problems in dask-awwkard.
We do this by adding a type_ argument to materialize everywhere and to maybe_materialize. maybe_materialize and layout.materialize() now materialize all instances of type_ and it defaults to MaterializableArray. For the special case of to_packed, we change this option to VirtualNDArray.

From the discussion in #3636, it was decided that this is the move for now.

@ikrommyd ikrommyd requested review from ianna and pfackeldey September 5, 2025 14:31
@codecov
Copy link
Copy Markdown

codecov bot commented Sep 5, 2025 •
edited
Loading

Codecov Report

❌ Patch coverage is 97.72727% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 82.67%. Comparing base (b749e49) to head (bd73966).
⚠️ Report is 416 commits behind head on main.

Files with missing lines Patch % Lines
src/awkward/record.py 0.00% 1 Missing ⚠️
Additional details and impacted files
Files with missing lines Coverage Δ
src/awkward/_nplikes/array_like.py 100.00% <100.00%> (+30.61%) ⬆️
src/awkward/contents/bitmaskedarray.py 69.85% <100.00%> (-0.59%) ⬇️
src/awkward/contents/bytemaskedarray.py 87.64% <100.00%> (-2.02%) ⬇️
src/awkward/contents/content.py 77.12% <100.00%> (+2.30%) ⬆️
src/awkward/contents/emptyarray.py 76.36% <100.00%> (+0.74%) ⬆️
src/awkward/contents/indexedarray.py 85.00% <100.00%> (+3.96%) ⬆️
src/awkward/contents/indexedoptionarray.py 89.10% <100.00%> (+0.89%) ⬆️
src/awkward/contents/listarray.py 90.55% <100.00%> (+1.12%) ⬆️
src/awkward/contents/listoffsetarray.py 81.09% <100.00%> (-1.78%) ⬇️
src/awkward/contents/numpyarray.py 91.35% <100.00%> (-0.16%) ⬇️
... and 6 more

... and 181 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@github-actions
Copy link
Copy Markdown

github-actions bot commented Sep 5, 2025

The documentation preview is ready to be viewed at http://preview.awkward-array.org.s3-website.us-east-1.amazonaws.com/PR3637

ianna
ianna approved these changes Sep 5, 2025
View reviewed changes
Copy link
Copy Markdown
Member

@ianna ianna left a comment

Choose a reason for hiding this comment

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

@ikrommyd - Great! Thanks for implementing it. I would have had a slight preference to "skip materializing placeholders" as a more concise title to this PR. As discussed - this should go into the release. Please merge if you have finished with it. Thanks!

@ianna ianna added the pr-next-release Required for the next release label Sep 5, 2025
@ikrommyd ikrommyd changed the title feat: do not materialize placeholders in to_packed feat: skip materialization of placeholders in to_packed Sep 5, 2025
@ikrommyd
Copy link
Copy Markdown
Collaborator Author

ikrommyd commented Sep 5, 2025

Thanks @ianna! Changed the title accordingly.
Waiting for @pfackeldey to approve too to make sure my eye didn't miss a place where I forgot the the type_ argument.
Will merge right afterwards.

pfackeldey
pfackeldey requested changes Sep 5, 2025
View reviewed changes
Copy link
Copy Markdown
Collaborator

@pfackeldey pfackeldey left a comment

Choose a reason for hiding this comment

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

I just have a typing suggestion to be more explicit here. Apart from that it looks great and ready to be merged!

(I forgot if we planned to add tests here, or not?)

@ikrommyd
Copy link
Copy Markdown
Collaborator Author

ikrommyd commented Sep 5, 2025

(I forgot if we planned to add tests here, or not?)

Well the problem arises in dask-awkward where the layout has ndarray and PlaceHolderArray buffers so I have tested it there. I guess we can add a test by manually creating such a hybrid layout.

@pfackeldey
Copy link
Copy Markdown
Collaborator

pfackeldey commented Sep 5, 2025

(I forgot if we planned to add tests here, or not?)

Well the problem arises in dask-awkward where the layout has ndarray and PlaceHolderArray buffers so I have tested it there. I guess we can add a test by manually creating such a hybrid layout.

yes, makes sense!

@ikrommyd
Copy link
Copy Markdown
Collaborator Author

ikrommyd commented Sep 5, 2025

@pfackeldey added the typing improvement and a test!

pfackeldey
pfackeldey approved these changes Sep 5, 2025
View reviewed changes
Copy link
Copy Markdown
Collaborator

@pfackeldey pfackeldey left a comment

Choose a reason for hiding this comment

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

Awesome! Thanks, please merge once the CI passes 👍

@ikrommyd
Copy link
Copy Markdown
Collaborator Author

ikrommyd commented Sep 5, 2025

Damn it windows 3.9 numpy dtypes 🤣 Let me enforce some dtype

@ikrommyd ikrommyd merged commit 3e82eef into scikit-hep:main Sep 5, 2025
46 checks passed
@ikrommyd ikrommyd deleted the do-not-materialize-in-to-packed branch September 5, 2025 18:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ianna ianna ianna approved these changes

@pfackeldey pfackeldey pfackeldey approved these changes

Assignees

No one assigned

Labels

pr-next-release Required for the next release

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@ikrommyd @pfackeldey @ianna