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.

Sign up for GitHub

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

Jump to bottom

fix: test untested path in ak.cartesian & broadcasting #2329

Merged
merged 13 commits into from
Mar 21, 2023
Merged

fix: test untested path in ak.cartesian & broadcasting #2329

merged 13 commits into from
Mar 21, 2023

Conversation

agoose77
Copy link
Collaborator

This PR:

  • removes a special case for zero-length arrays from ak.cartesian
  • fixes missing case in broadcast_and_apply that required this patch
  • fixes and tests a set of inputs that was broken in ak.cartesian

@agoose77
Copy link
Collaborator Author

@jpivarski as this touches broadcasting code, I'd appreciate your eyes to take a close look at the changes to _broadcasting.py in order to assert that I've not introduced a regression. It looks like we don't actually pull out the contents of RegularArray, which we should be doing according the context of the branch (given that all_same_offsets checks the RegularArray offsets).

@agoose77 agoose77 requested a review from jpivarski March 21, 2023 14:04
@agoose77 agoose77 temporarily deployed to docs-preview March 21, 2023 14:20 — with GitHub Actions Inactive
@codecov
Copy link

codecov bot commented Mar 21, 2023 •
edited
Loading

Codecov Report

Merging #2329 (da5c4fa) into main (6adf9a4) will decrease coverage by 0.02%.
The diff coverage is 96.92%.

Additional details and impacted files
Impacted Files Coverage Δ
src/awkward/index.py 90.06% <83.33%> (-0.39%) ⬇️
src/awkward/operations/ak_cartesian.py 92.00% <98.24%> (+1.02%) ⬆️
src/awkward/_broadcasting.py 89.33% <100.00%> (+0.04%) ⬆️

agoose77
agoose77 commented Mar 21, 2023
View reviewed changes
src/awkward/_broadcasting.py
@@ -160,7 +160,7 @@ def all_same_offsets(backend: ak._backends.Backend, inputs: list) -> bool:
my_offsets = index_nplike.empty(0, dtype=np.int64)
else:
my_offsets = index_nplike.arange(
0, x.content.length, x.size, dtype=np.int64
0, x.content.length + 1, x.size, dtype=np.int64
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We want a range that is inclusive of the endpoints, which is not the case by default.

Copy link
Member

Choose a reason for hiding this comment

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

We do? Oh, yes—we do! Offsets always have to be one longer than the array itself; that's right. Good catch!

agoose77
agoose77 commented Mar 21, 2023
View reviewed changes
src/awkward/_broadcasting.py
Comment on lines +844 to +845
elif isinstance(x, RegularArray):
nextinputs.append(x.content[: x.size * x.length])
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We test for RegularArray in all_same_offsets, but here we weren't actually pulling out the content.

@agoose77 agoose77 temporarily deployed to docs-preview March 21, 2023 14:45 — with GitHub Actions Inactive
jpivarski
jpivarski approved these changes Mar 21, 2023
View reviewed changes
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.

Fantastic; I'm glad you found this.

src/awkward/_broadcasting.py
@@ -160,7 +160,7 @@ def all_same_offsets(backend: ak._backends.Backend, inputs: list) -> bool:
my_offsets = index_nplike.empty(0, dtype=np.int64)
else:
my_offsets = index_nplike.arange(
0, x.content.length, x.size, dtype=np.int64
0, x.content.length + 1, x.size, dtype=np.int64
Copy link
Member

Choose a reason for hiding this comment

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

We do? Oh, yes—we do! Offsets always have to be one longer than the array itself; that's right. Good catch!

@agoose77 agoose77 merged commit a3e77b7 into main Mar 21, 2023
@agoose77 agoose77 deleted the agoose77/fix-cartesian-special-case branch March 21, 2023 15:07
@agoose77 agoose77 mentioned this pull request Mar 21, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jpivarski jpivarski jpivarski 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.

None yet
2 participants
@agoose77 @jpivarski