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: guard broadcast_and_apply from mixed backends #2678

Merged
merged 8 commits into from
Aug 29, 2023

Conversation

agoose77
Copy link
Collaborator

@agoose77 agoose77 commented Aug 29, 2023

This PR ensures that broadcast_and_apply throws an error if the given arrays do not share the same backend.

At a high level, all ak.XXX operations that accept multiple array arguments should effectively compute

backend = backend_of(*args, default=..., coerce_to_common=True)
layouts = [ak.to_layout(x).to_backend(backend) for x in args]

We could add a helper to do this, although given that each ak.XXX function needs to set different layout conversion flags, this is probably best avoided.

There's an argument to be made for making broadcast_and_apply be more permissive and perform the coercion itself. However, I think we want to coerce to the same backend in the highlevel ak.XXX functions ASAP, i.e. before broadcasting.

Whilst I was here, I cleaned up some of the routines that use backend_of.

@codecov
Copy link

codecov bot commented Aug 29, 2023

Codecov Report

Merging #2678 (e1ea6cc) into main (dea0139) will increase coverage by 0.01%.
The diff coverage is 94.53%.

Additional details and impacted files
Files Changed Coverage Δ
src/awkward/operations/ak_to_backend.py 100.00% <ø> (ø)
src/awkward/_broadcasting.py 95.43% <50.00%> (-0.21%) ⬇️
src/awkward/_backends/dispatch.py 92.85% <88.23%> (-1.18%) ⬇️
src/awkward/operations/str/akstr_join.py 93.33% <90.90%> (+0.65%) ⬆️
src/awkward/operations/str/akstr_repeat.py 95.12% <92.85%> (+0.67%) ⬆️
src/awkward/operations/ak_where.py 92.59% <93.75%> (-0.14%) ⬇️
src/awkward/_connect/numpy.py 91.89% <100.00%> (+0.07%) ⬆️
src/awkward/contents/content.py 76.16% <100.00%> (ø)
src/awkward/operations/ak_almost_equal.py 93.18% <100.00%> (+0.49%) ⬆️
src/awkward/operations/ak_argcartesian.py 88.57% <100.00%> (ø)
... and 20 more

@agoose77 agoose77 temporarily deployed to docs-preview August 29, 2023 11:37 — with GitHub Actions Inactive
@agoose77 agoose77 temporarily deployed to docs-preview August 29, 2023 11:55 — with GitHub Actions Inactive
@agoose77 agoose77 temporarily deployed to docs-preview August 29, 2023 12:45 — with GitHub Actions Inactive
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.

This all looks good. Thanks!

@agoose77 agoose77 merged commit cd32010 into main Aug 29, 2023
34 checks passed
@agoose77 agoose77 deleted the agoose77/fix-guard-broadcast-backend branch August 29, 2023 15:02
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