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: support non int64 index in ListArray's pad_none #2634

Merged
merged 3 commits into from
Aug 10, 2023

Conversation

agoose77
Copy link
Collaborator

@agoose77 agoose77 commented Aug 10, 2023

This PR does two things:

  1. Changes dtypes of a kernel call (awkward_ListArray_rpad_axis1)
    The kernel is written with the following template signature
    T* toindex,
    const C* fromstarts,
    const C* fromstops,
    C* tostarts,
    C* tostops,
    i.e. starts and tostarts are expected to have the same dtypes. We can rewrite the kernel call site to ensure that this is true. This bug was observed after reading data from Arrow with 32-bit indices.
  2. When using pad_none, we shouldn't copy the parameters of the content to the new mask node; this duplicates parameters, and doesn't consider the rules about e.g. whether the content is allowed to possess such parameters.1

Footnotes

  1. this remains a general class of problem IIRC with broadcasting that we'll need to work on. However, this fix is also correct; we don't want duplicate parameters.

@codecov
Copy link

codecov bot commented Aug 10, 2023

Codecov Report

Merging #2634 (746cbdb) into main (e2eb69a) will not change coverage.
The diff coverage is 100.00%.

Additional details and impacted files
Files Changed Coverage Δ
src/awkward/contents/content.py 76.16% <100.00%> (ø)
src/awkward/contents/listarray.py 88.60% <100.00%> (ø)

@agoose77 agoose77 temporarily deployed to docs-preview August 10, 2023 10:20 — with GitHub Actions Inactive
@agoose77 agoose77 marked this pull request as ready for review August 10, 2023 10:23
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.

Good catch; we use 64-bit so often that it's easy to miss cases that should work for 32-bit as well.

I'll merge it.

@jpivarski jpivarski merged commit 8ba3e30 into main Aug 10, 2023
36 checks passed
@jpivarski jpivarski deleted the agoose77/fix-listarray-kernel branch August 10, 2023 16:43
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.

None yet

2 participants