fix: simplify `ListOffsetArray_reduce_nonlocal_outstartsstops` #1796

agoose77 · 2022-10-17T14:57:11Z

fixes Segmentation fault with nested pair-wise comparison #1784
fixeds Reduction of inner dimension in ragged array produces incorrect offsets #1791
removes unused kernel
adds test

This PR fixes #1784 by rewriting the awkward_ListOffsetArray_reduce_nonlocal_outstartsstops_64 kernel. The actual fix for #1784 is just to wrap the entire kernel body in the if (distincts[i] != -1) guard. However, I took the opportunity to simplify and explain the kernel. This means that we can drop the gaps kernel, and we don't need to perform any float divisions. So, I'd hope that this is also a slight performance boost (but this did not motivate the rewrite).

I hope that I haven't overlooked any design invariants or corner-cases 🤞; I've attempted to understand the purpose and function of the kernel in depth before writing this PR.

⚠️ I did change the expected outcomes of the test suite - some of the kernel tests check the offset values. We don't care about the actual values of starts and stops as long as they're in-bounds and equal.

codecov · 2022-10-17T16:13:41Z

Codecov Report

Merging #1796 (9c1f687) into main (eee968a) will decrease coverage by 0.01%.
The diff coverage is n/a.

Additional details and impacted files

Impacted Files	Coverage Δ
src/awkward/contents/listoffsetarray.py	`79.53% <ø> (-0.12%)`	⬇️
src/awkward/highlevel.py	`75.87% <ø> (ø)`
src/awkward/contents/indexedarray.py	`79.76% <0.00%> (-0.19%)`	⬇️
src/awkward/contents/recordarray.py	`83.72% <0.00%> (-0.18%)`	⬇️
src/awkward/_util.py	`82.36% <0.00%> (-0.18%)`	⬇️
src/awkward/contents/indexedoptionarray.py	`88.92% <0.00%> (-0.16%)`	⬇️
src/awkward/_connect/avro.py	`87.17% <0.00%> (-0.14%)`	⬇️
src/awkward/types/_awkward_datashape_parser.py	`47.72% <0.00%> (-0.01%)`	⬇️
... and 1 more

jpivarski

I'm surprised that it's possible to solve this problem without the gaps, though gaps was added to handle a special case, after the original algorithm was first written. There are probably tests for it in the suite, but it's hard to tell which ones would be relevant, so I wrote a new test that is sensitive to what the gaps are trying to accomplish.

First, I found a test that has non-trivial gaps. Then I turned it into a commandline example for flexibility. Everything between the next two horizontal lines is in main (with gaps), to get an unchanged baseline. The commandline prints the original array layout followed by the computed output.

I modified the code to print the gaps on line listoffsetarray.py:1510, right after they are defined. The size of the gaps array is large enough for the maximum that gaps can be, so (in this example) the last item is undefined.

Product of irregular prime numbers, two ListOffsetArrays deep.

% python -c 'import awkward as ak, numpy as np; array = ak.Array(ak.contents.ListOffsetArray(ak.index.Index64([0, 4, 4, 6]), ak.contents.ListOffsetArray(ak.index.Index64([0, 3, 3, 5, 6, 8, 9]), ak.contents.NumpyArray([ 2,  3,  5,  7, 11, 13, 17, 19, 23])))); print(array.layout); print(ak.Array(ak.prod(array, axis=-2), check_valid=True))'

<ListOffsetArray len='3'>
    <offsets><Index dtype='int64' len='4'>[0 4 4 6]</Index></offsets>
    <content><ListOffsetArray len='6'>
        <offsets><Index dtype='int64' len='7'>[0 3 3 5 6 8 9]</Index></offsets>
        <content><NumpyArray dtype='int64' len='9'>[ 2  3  5  7 11 13 17 19 23]</NumpyArray></content>
    </ListOffsetArray></content>
</ListOffsetArray>
gaps [                  1                   2 4607182418800017408]
[[182, 33, 5], [], [391, 19]]

([[182, 33, 5], [], [391, 19]] is the correct answer.)

Now the ListOffsetArrays don't start at 0. There's extra junk in the contents before the real lists start.

% python -c 'import awkward as ak, numpy as np; array = ak.Array(ak.contents.ListOffsetArray(ak.index.Index64(np.array([0, 4, 4, 6]) + 2), ak.contents.ListOffsetArray(ak.index.Index64(np.array([123, 321, 0, 3, 3, 5, 6, 8, 9]) + 1), ak.contents.NumpyArray([9999, 2,  3,  5,  7, 11, 13, 17, 19, 23])))); print(array.layout); print(ak.Array(ak.prod(array, axis=-2), check_valid=True))'

<ListOffsetArray len='3'>
    <offsets><Index dtype='int64' len='4'>[2 6 6 8]</Index></offsets>
    <content><ListOffsetArray len='8'>
        <offsets><Index dtype='int64' len='9'>
            [124 322   1   4   4   6   7   9  10]
        </Index></offsets>
        <content><NumpyArray dtype='int64' len='10'>
            [9999    2    3    5    7   11   13   17   19   23]
        </NumpyArray></content>
    </ListOffsetArray></content>
</ListOffsetArray>
gaps [                  1                   2 4607182418800017408]
[[182, 33, 5], [], [391, 19]]

Now they're ListArrays, rather than ListOffsetArrays.

% python -c 'import awkward as ak, numpy as np; array = ak.Array(ak.contents.ListArray(ak.index.Index64([0, 4, 4]), ak.index.Index64([4, 4, 6]), ak.contents.ListArray(ak.index.Index64([0, 3, 3, 5, 6, 8]), ak.index.Index64([3, 3, 5, 6, 8, 9]), ak.contents.NumpyArray([ 2,  3,  5,  7, 11, 13, 17, 19, 23])))); print(array.layout); print(ak.Array(ak.prod(array, axis=-2), check_valid=True))'

<ListArray len='3'>
    <starts><Index dtype='int64' len='3'>[0 4 4]</Index></starts>
    <stops><Index dtype='int64' len='3'>[4 4 6]</Index></stops>
    <content><ListArray len='6'>
        <starts><Index dtype='int64' len='6'>[0 3 3 5 6 8]</Index></starts>
        <stops><Index dtype='int64' len='6'>[3 3 5 6 8 9]</Index></stops>
        <content><NumpyArray dtype='int64' len='9'>[ 2  3  5  7 11 13 17 19 23]</NumpyArray></content>
    </ListArray></content>
</ListArray>
gaps [                  1                   2 4612217596255138984]
[[182, 33, 5], [], [391, 19]]

Now the cases in which starts[i] == stops[i] are replaced with crazy values, which is legal. (If starts[i] == stops[i], the list is considered empty. Neither starts[i] nor stops[i] have to be sane.)

% python -c 'import awkward as ak, numpy as np; array = ak.Array(ak.contents.ListArray(ak.index.Index64([0, 100, 4]), ak.index.Index64([4, 100, 6]), ak.contents.ListArray(ak.index.Index64([0, 1000, 3, 5, 6, 8]), ak.index.Index64([3, 1000, 5, 6, 8, 9]), ak.contents.NumpyArray([ 2,  3,  5,  7, 11, 13, 17, 19, 23])))); print(array.layout); print(ak.Array(ak.prod(array, axis=-2), check_valid=True))'

<ListArray len='3'>
    <starts><Index dtype='int64' len='3'>[  0 100   4]</Index></starts>
    <stops><Index dtype='int64' len='3'>[  4 100   6]</Index></stops>
    <content><ListArray len='6'>
        <starts><Index dtype='int64' len='6'>
            [   0 1000    3    5    6    8]
        </Index></starts>
        <stops><Index dtype='int64' len='6'>
            [   3 1000    5    6    8    9]
        </Index></stops>
        <content><NumpyArray dtype='int64' len='9'>[ 2  3  5  7 11 13 17 19 23]</NumpyArray></content>
    </ListArray></content>
</ListArray>
gaps [                  1                   2 4612217596255138984]
[[182, 33, 5], [], [391, 19]]

Now there's junk data between some of the valid items, but the starts and stops skip it.

% python -c 'import awkward as ak, numpy as np; array = ak.Array(ak.contents.ListArray(ak.index.Index64([0, 100, 6]), ak.index.Index64([4, 100, 8]), ak.contents.ListArray(ak.index.Index64([0, 1000, 3, 5, 60, 80, 6, 8]), ak.index.Index64([3, 1000, 5, 6, 80, 90, 8, 9]), ak.contents.NumpyArray([ 2,  3,  5,  7, 11, 13, 17, 19, 23])))); print(array.layout); print(ak.Array(ak.prod(array, axis=-2), check_valid=True))'

<ListArray len='3'>
    <starts><Index dtype='int64' len='3'>[  0 100   6]</Index></starts>
    <stops><Index dtype='int64' len='3'>[  4 100   8]</Index></stops>
    <content><ListArray len='8'>
        <starts><Index dtype='int64' len='8'>
            [   0 1000    3    5   60   80    6    8]
        </Index></starts>
        <stops><Index dtype='int64' len='8'>
            [   3 1000    5    6   80   90    8    9]
        </Index></stops>
        <content><NumpyArray dtype='int64' len='9'>[ 2  3  5  7 11 13 17 19 23]</NumpyArray></content>
    </ListArray></content>
</ListArray>
gaps [                  1                   2 4612217596255138984]
[[182, 33, 5], [], [391, 19]]

Now one of the items is actually out of order (the first 3 items of the NumpyArray have been moved to the end).

% python -c 'import awkward as ak, numpy as np; array = ak.Array(ak.contents.ListArray(ak.index.Index64([0, 100, 6]), ak.index.Index64([4, 100, 8]), ak.contents.ListArray(ak.index.Index64([9, 1000, 3, 5, 60, 80, 6, 8]), ak.index.Index64([12, 1000, 5, 6, 80, 90, 8, 9]), ak.contents.NumpyArray([ 20,  30,  50,  7, 11, 13, 17, 19, 23, 2, 3, 5])))); print(array.layout); print(ak.Array(ak.prod(array, axis=-2), check_valid=True))'

<ListArray len='3'>
    <starts><Index dtype='int64' len='3'>[  0 100   6]</Index></starts>
    <stops><Index dtype='int64' len='3'>[  4 100   8]</Index></stops>
    <content><ListArray len='8'>
        <starts><Index dtype='int64' len='8'>
            [   9 1000    3    5   60   80    6    8]
        </Index></starts>
        <stops><Index dtype='int64' len='8'>
            [  12 1000    5    6   80   90    8    9]
        </Index></stops>
        <content><NumpyArray dtype='int64' len='12'>
            [20 30 50  7 11 13 17 19 23  2  3  5]
        </NumpyArray></content>
    </ListArray></content>
</ListArray>
gaps [                  1                   2 4612217596255138984]
[[182, 33, 5], [], [391, 19]]

In each of the above cases, the high-level view of the input array is the same, though the layout differs.

Okay, now same thing, but in this PR's branch. I removed the print-out in Python and added one to the kernel code to be sure that I'm running the new kernel code. (Everything was recompiled.)

% python -c 'import awkward as ak, numpy as np; array = ak.Array(ak.contents.ListOffsetArray(ak.index.Index64([0, 4, 4, 6]), ak.contents.ListOffsetArray(ak.index.Index64([0, 3, 3, 5, 6, 8, 9]), ak.contents.NumpyArray([ 2,  3,  5,  7, 11, 13, 17, 19, 23])))); print(array.layout); print(ak.Array(ak.prod(array, axis=-2), check_valid=True))'

<ListOffsetArray len='3'>
    <offsets><Index dtype='int64' len='4'>[0 4 4 6]</Index></offsets>
    <content><ListOffsetArray len='6'>
        <offsets><Index dtype='int64' len='7'>[0 3 3 5 6 8 9]</Index></offsets>
        <content><NumpyArray dtype='int64' len='9'>[ 2  3  5  7 11 13 17 19 23]</NumpyArray></content>
    </ListOffsetArray></content>
</ListOffsetArray>
no gaps
[[182, 33, 5], [], [391, 19]]

% python -c 'import awkward as ak, numpy as np; array = ak.Array(ak.contents.ListOffsetArray(ak.index.Index64(np.array([0, 4, 4, 6]) + 2), ak.contents.ListOffsetArray(ak.index.Index64(np.array([123, 321, 0, 3, 3, 5, 6, 8, 9]) + 1), ak.contents.NumpyArray([9999, 2,  3,  5,  7, 11, 13, 17, 19, 23])))); print(array.layout); print(ak.Array(ak.prod(array, axis=-2), check_valid=True))'

<ListOffsetArray len='3'>
    <offsets><Index dtype='int64' len='4'>[2 6 6 8]</Index></offsets>
    <content><ListOffsetArray len='8'>
        <offsets><Index dtype='int64' len='9'>
            [124 322   1   4   4   6   7   9  10]
        </Index></offsets>
        <content><NumpyArray dtype='int64' len='10'>
            [9999    2    3    5    7   11   13   17   19   23]
        </NumpyArray></content>
    </ListOffsetArray></content>
</ListOffsetArray>
no gaps
[[182, 33, 5], [], [391, 19]]

% python -c 'import awkward as ak, numpy as np; array = ak.Array(ak.contents.ListArray(ak.index.Index64([0, 4, 4]), ak.index.Index64([4, 4, 6]), ak.contents.ListArray(ak.index.Index64([0, 3, 3, 5, 6, 8]), ak.index.Index64([3, 3, 5, 6, 8, 9]), ak.contents.NumpyArray([ 2,  3,  5,  7, 11, 13, 17, 19, 23])))); print(array.layout); print(ak.Array(ak.prod(array, axis=-2), check_valid=True))'

<ListArray len='3'>
    <starts><Index dtype='int64' len='3'>[0 4 4]</Index></starts>
    <stops><Index dtype='int64' len='3'>[4 4 6]</Index></stops>
    <content><ListArray len='6'>
        <starts><Index dtype='int64' len='6'>[0 3 3 5 6 8]</Index></starts>
        <stops><Index dtype='int64' len='6'>[3 3 5 6 8 9]</Index></stops>
        <content><NumpyArray dtype='int64' len='9'>[ 2  3  5  7 11 13 17 19 23]</NumpyArray></content>
    </ListArray></content>
</ListArray>
no gaps
[[182, 33, 5], [], [391, 19]]

% python -c 'import awkward as ak, numpy as np; array = ak.Array(ak.contents.ListArray(ak.index.Index64([0, 100, 4]), ak.index.Index64([4, 100, 6]), ak.contents.ListArray(ak.index.Index64([0, 1000, 3, 5, 6, 8]), ak.index.Index64([3, 1000, 5, 6, 8, 9]), ak.contents.NumpyArray([ 2,  3,  5,  7, 11, 13, 17, 19, 23])))); print(array.layout); print(ak.Array(ak.prod(array, axis=-2), check_valid=True))'

<ListArray len='3'>
    <starts><Index dtype='int64' len='3'>[  0 100   4]</Index></starts>
    <stops><Index dtype='int64' len='3'>[  4 100   6]</Index></stops>
    <content><ListArray len='6'>
        <starts><Index dtype='int64' len='6'>
            [   0 1000    3    5    6    8]
        </Index></starts>
        <stops><Index dtype='int64' len='6'>
            [   3 1000    5    6    8    9]
        </Index></stops>
        <content><NumpyArray dtype='int64' len='9'>[ 2  3  5  7 11 13 17 19 23]</NumpyArray></content>
    </ListArray></content>
</ListArray>
no gaps
[[182, 33, 5], [], [391, 19]]

% python -c 'import awkward as ak, numpy as np; array = ak.Array(ak.contents.ListArray(ak.index.Index64([0, 100, 6]), ak.index.Index64([4, 100, 8]), ak.contents.ListArray(ak.index.Index64([0, 1000, 3, 5, 60, 80, 6, 8]), ak.index.Index64([3, 1000, 5, 6, 80, 90, 8, 9]), ak.contents.NumpyArray([ 2,  3,  5,  7, 11, 13, 17, 19, 23])))); print(array.layout); print(ak.Array(ak.prod(array, axis=-2), check_valid=True))'

<ListArray len='3'>
    <starts><Index dtype='int64' len='3'>[  0 100   6]</Index></starts>
    <stops><Index dtype='int64' len='3'>[  4 100   8]</Index></stops>
    <content><ListArray len='8'>
        <starts><Index dtype='int64' len='8'>
            [   0 1000    3    5   60   80    6    8]
        </Index></starts>
        <stops><Index dtype='int64' len='8'>
            [   3 1000    5    6   80   90    8    9]
        </Index></stops>
        <content><NumpyArray dtype='int64' len='9'>[ 2  3  5  7 11 13 17 19 23]</NumpyArray></content>
    </ListArray></content>
</ListArray>
no gaps
[[182, 33, 5], [], [391, 19]]

% python -c 'import awkward as ak, numpy as np; array = ak.Array(ak.contents.ListArray(ak.index.Index64([0, 100, 6]), ak.index.Index64([4, 100, 8]), ak.contents.ListArray(ak.index.Index64([9, 1000, 3, 5, 60, 80, 6, 8]), ak.index.Index64([12, 1000, 5, 6, 80, 90, 8, 9]), ak.contents.NumpyArray([ 20,  30,  50,  7, 11, 13, 17, 19, 23, 2, 3, 5])))); print(array.layout); print(ak.Array(ak.prod(array, axis=-2), check_valid=True))'

<ListArray len='3'>
    <starts><Index dtype='int64' len='3'>[  0 100   6]</Index></starts>
    <stops><Index dtype='int64' len='3'>[  4 100   8]</Index></stops>
    <content><ListArray len='8'>
        <starts><Index dtype='int64' len='8'>
            [   9 1000    3    5   60   80    6    8]
        </Index></starts>
        <stops><Index dtype='int64' len='8'>
            [  12 1000    5    6   80   90    8    9]
        </Index></stops>
        <content><NumpyArray dtype='int64' len='12'>
            [20 30 50  7 11 13 17 19 23  2  3  5]
        </NumpyArray></content>
    </ListArray></content>
</ListArray>
no gaps
[[182, 33, 5], [], [391, 19]]

Awesome!

In retrospect, these all had to work because all of the layouts are normalized through toListOffsetArray64(True) before the calculation with the gaps would have begun. So this was actually testing the toListOffsetArray64(True) function, but that's a prerequisite for the code you changed.

Anyway, even if these 6 tests were really the same test of gaps, we do know that it has been tested with non-trivial gaps. So I don't see how there could be any other corner cases to check.

I'll just add them to the test suite and enable auto-merge.

jpivarski · 2022-10-17T21:05:02Z

src/cpu-kernels/awkward_ListOffsetArray_reduce_nonlocal_outstartsstops_64.cpp

-#define FILENAME(line) FILENAME_FOR_EXCEPTIONS_C("src/cpu-kernels/awkward_ListOffsetArray_reduce_nonlocal_outstartsstops_64.cpp", line)
-
+#define FILENAME(line)                                                 \
+  FILENAME_FOR_EXCEPTIONS_C(                                           \
+      "src/cpu-kernels/"                                               \
+      "awkward_ListOffsetArray_reduce_nonlocal_outstartsstops_64.cpp", \
+      line)


These were all in one line because they're easier to maintain with text-based tools (awk, sed, grep), but we've never needed to do that after their initial generation, so I doubt there will be any issues with this reformatting.

jpivarski · 2022-10-17T22:37:43Z

⚠️ I did change the expected outcomes of the test suite - some of the kernel tests check the offset values.

Those test outcomes were set by running the code itself, so the new values are just as good as the old ones. They were specifically the auto-generated unit tests, not the high-level tests we write by hand in the tests directory.

agoose77 added 3 commits October 16, 2022 00:30

test: add test for #1791

c1e0184

fix: fix #1791

7621e18

fix: simplify reduce_nonlocal_outstartstops_64.cpp

b735ca1

agoose77 changed the title ~~fix: simplify ListOffsetArray_reduce_nonlocal_nextshifts~~ fix: simplify ListOffsetArray_reduce_nonlocal_outstartsstops Oct 17, 2022

agoose77 added 2 commits October 17, 2022 16:06

chore: remove unused kernel

86ddf7e

fix: remove test-data for gaps

7ee7233

This was referenced Oct 17, 2022

Segmentation fault with nested pair-wise comparison #1784

Closed

fix: produce valid offsets for reducing with empty trailing lists #1792

Closed

agoose77 added 4 commits October 17, 2022 16:16

test: validate fix for leading sublists

585234c

test: explicitly check starts and stops

64887c8

fix: handle empty arrays (and all empty sublists)

1815984

test: update test specifications

d968ec6

agoose77 requested a review from jpivarski October 17, 2022 16:34

agoose77 mentioned this pull request Oct 17, 2022

fix: simplify ListOffsetArray_reduce_nonlocal_outstartsstops (backport) #1797

Merged

agoose77 added the pr-needs-backport This PR needs a counterpart to backport to older versions label Oct 17, 2022

jpivarski approved these changes Oct 17, 2022

View reviewed changes

jpivarski and others added 2 commits October 17, 2022 17:21

Adding the review tests to the permanent test suite.

077a081

Merge branch 'main' into agoose77/fix-outstartstops

9c1f687

jpivarski enabled auto-merge (squash) October 17, 2022 22:22

jpivarski merged commit 65f9fcd into main Oct 17, 2022

jpivarski deleted the agoose77/fix-outstartstops branch October 17, 2022 22:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: simplify `ListOffsetArray_reduce_nonlocal_outstartsstops` #1796

fix: simplify `ListOffsetArray_reduce_nonlocal_outstartsstops` #1796

agoose77 commented Oct 17, 2022 •

edited

Loading

codecov bot commented Oct 17, 2022 •

edited

Loading

jpivarski left a comment

jpivarski Oct 17, 2022

jpivarski commented Oct 17, 2022

fix: simplify ListOffsetArray_reduce_nonlocal_outstartsstops #1796

fix: simplify ListOffsetArray_reduce_nonlocal_outstartsstops #1796

Conversation

agoose77 commented Oct 17, 2022 • edited Loading

codecov bot commented Oct 17, 2022 • edited Loading

Codecov Report

jpivarski left a comment

Choose a reason for hiding this comment

jpivarski Oct 17, 2022

Choose a reason for hiding this comment

jpivarski commented Oct 17, 2022

fix: simplify `ListOffsetArray_reduce_nonlocal_outstartsstops` #1796

fix: simplify `ListOffsetArray_reduce_nonlocal_outstartsstops` #1796

agoose77 commented Oct 17, 2022 •

edited

Loading

codecov bot commented Oct 17, 2022 •

edited

Loading