Skip to content

fix: use jax's __setitem__ syntax in ak.merge_union_of_records to make it work with the jax backend#3596

Merged
ianna merged 3 commits intoscikit-hep:mainfrom
ikrommyd:merge-union-of-records-fix
Oct 24, 2025
Merged

fix: use jax's __setitem__ syntax in ak.merge_union_of_records to make it work with the jax backend#3596
ianna merged 3 commits intoscikit-hep:mainfrom
ikrommyd:merge-union-of-records-fix

Conversation

@ikrommyd
Copy link
Copy Markdown
Collaborator

@ikrommyd ikrommyd commented Aug 3, 2025 •
edited
Loading

ak.merge_union_of_records doesn't currently handle jax's setitem syntax. I'm adding that

@ikrommyd ikrommyd requested a review from pfackeldey August 3, 2025 07:30
@codecov
Copy link
Copy Markdown

codecov bot commented Aug 3, 2025 •
edited
Loading

Codecov Report

❌ Patch coverage is 63.63636% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 82.72%. Comparing base (b749e49) to head (90f6670).
⚠️ Report is 452 commits behind head on main.

Files with missing lines Patch % Lines
...rc/awkward/operations/ak_merge_union_of_records.py 63.63% 4 Missing ⚠️
Additional details and impacted files
Files with missing lines Coverage Δ
...rc/awkward/operations/ak_merge_union_of_records.py 82.11% <63.63%> (-3.48%) ⬇️

... and 199 files with indirect coverage changes

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

@ikrommyd ikrommyd requested a review from ianna August 3, 2025 09:00
ianna
ianna requested changes Aug 3, 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 - thanks for looking into it. As we have discussed during the last meeting, we are refactoring the JAX, VirualArrays, PlaceholerArrays, and TypeTracerArrays code. IMHO, it would be better to have it all in one place. Please, add this to the list of issues to be fixed. Thanks!

@ikrommyd
Copy link
Copy Markdown
Collaborator Author

ikrommyd commented Aug 3, 2025

Well refactoring anything doesn’t touch this. This is just an operation that somewhat accidentally works with cpu but doesn’t work with anything else so I just fixed that. It’s not a super common operation though so please take over the PR as you please. It’s not critical.

@ikrommyd
Copy link
Copy Markdown
Collaborator Author

ikrommyd commented Aug 3, 2025

Given #3598, I'm chaging this to target the jax branch and setting it to draft mode to be merged later.

@ikrommyd ikrommyd changed the base branch from main to awkward-jax August 3, 2025 14:37
@ikrommyd ikrommyd marked this pull request as draft August 3, 2025 14:37
@ikrommyd ikrommyd changed the title fix: make ak.merge_union_of_records work with other backends apart from numpy fix: make ak.merge_union_of_records work with the jax backend Aug 3, 2025
@ikrommyd ikrommyd changed the title fix: make ak.merge_union_of_records work with the jax backend fix: use jax's __setitem__ syntax in ak.merge_union_of_records to make it work with the jax backend Aug 3, 2025
@ianna
Copy link
Copy Markdown
Member

ianna commented Aug 4, 2025

Given #3598, I'm chaging this to target the jax branch and setting it to draft mode to be merged later.

Thanks, as discussed, we are removing both Jax nplike and Jax backend. Please, move this to your fork if your development is based on it. Thanks.

@ikrommyd
Copy link
Copy Markdown
Collaborator Author

ikrommyd commented Aug 4, 2025 •
edited
Loading

It already lives on my fork. It now targets the awkward-jax branch and not main which I believe makes this a valid PR if jax backend is only to live there after the removal.

@ianna
Copy link
Copy Markdown
Member

ianna commented Aug 4, 2025

It already lives on my fork. It now targets the awkward-jax branch and not main which I believe makes this a valid PR if jax backend is only to live there after the removal.

AFAIK we have no plans to maintain the awkward-jax branch. You can merge it to your fork.

@ianna
Copy link
Copy Markdown
Member

ianna commented Aug 4, 2025

It already lives on my fork. It now targets the awkward-jax branch and not main which I believe makes this a valid PR if jax backend is only to live there after the removal.

It should not be on this list. Please move it to your fork. I’m closing here. Thanks!

@ianna ianna closed this Aug 4, 2025
@ikrommyd ikrommyd reopened this Oct 24, 2025
@ikrommyd ikrommyd changed the base branch from awkward-jax to main October 24, 2025 05:06
@ikrommyd ikrommyd marked this pull request as ready for review October 24, 2025 05:06
@github-actions
Copy link
Copy Markdown

github-actions bot commented Oct 24, 2025

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

@ikrommyd
Copy link
Copy Markdown
Collaborator Author

ikrommyd commented Oct 24, 2025

@ianna this is the PR I meant today. Just fixes these couple of setitem calls.

ianna
ianna approved these changes Oct 24, 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! Thank you.

@ianna
Copy link
Copy Markdown
Member

ianna commented Oct 24, 2025

@pfackeldey - ping :-)

pfackeldey
pfackeldey approved these changes Oct 24, 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.

LGTM! 👍

@ianna ianna merged commit 580a2c1 into scikit-hep:main Oct 24, 2025
41 checks passed
@ikrommyd ikrommyd mentioned this pull request Oct 24, 2025
Merged
@ikrommyd ikrommyd deleted the merge-union-of-records-fix branch October 24, 2025 23:13
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

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@ikrommyd @ianna @pfackeldey