Fix: support nested option types in ak.is_none

[agoose77]

Fixes #1193
Requires #1279

To implement this feature, I introduce a temporary union. Let me know if there's a more elegant way to do this! Given that, unlike most other layout transformers, we need to modify several layers, I've split the transformation process into two different transforms - one to locate the first layout, another to actually perform the transformation.

I also wondered whether it would be possible to write the new recursive getfunctions with an early-return style instead of explicitly handling each case? In the past, the return value had a lot of interpretation modes, so it was more dangerous. With v2, I wonder if we could re-consider this position?

[agoose77]

NB - I haven't enabled any tests that currently skip due to missing is_none - I noticed that one failed, and this is all the time I have to look at this for now.

[codecov]

Codecov Report

Merging #1249 (8814309) into main (8e68200) will increase coverage by 1.76%.
The diff coverage is 79.72%.

Impacted Files	Coverage Δ
src/awkward/_v2/_connect/numba/arrayview.py	96.24% <ø> (ø)
src/awkward/_v2/_connect/numba/layout.py	87.01% <ø> (ø)
src/awkward/_v2/contents/bytemaskedarray.py	81.09% <9.09%> (-1.96%)	⬇️
src/awkward/_v2/contents/indexedarray.py	59.17% <10.00%> (-1.47%)	⬇️
src/awkward/_v2/contents/unmaskedarray.py	55.25% <20.00%> (-0.83%)	⬇️
src/awkward/_v2/operations/io/ak_to_json_file.py	47.05% <43.75%> (-27.95%)	⬇️
src/awkward/_v2/contents/emptyarray.py	69.14% <50.00%> (-0.23%)	⬇️
src/awkward/_v2/forms/emptyform.py	77.35% <50.00%> (-1.08%)	⬇️
src/awkward/_v2/contents/content.py	81.59% <58.33%> (-0.47%)	⬇️
src/awkward/_v2/contents/regulararray.py	81.97% <58.82%> (-0.88%)	⬇️
... and 39 more

[jpivarski]

To implement this feature, I introduce a temporary union. Let me know if there's a more elegant way to do this! Given that, unlike most other layout transformers, we need to modify several layers, I've split the transformation process into two different transforms - one to locate the first layout, another to actually perform the transformation.

Temporary UnionArrays are fine; they're used in a few other cases.

One transformation/action invoking another is also precedented. In fact, if we know that it can go exactly two levels of transforming deep, then I think it's more readable this way than trying to feed it into itself with a different parameter, just because having different states in a finite state machine be laid out as different code on the screen is easier on human brains. "Action 1: locate, action 2: transform" is very clear.

I also wondered whether it would be possible to write the new recursive getfunctions with an early-return style instead of explicitly handling each case? In the past, the return value had a lot of interpretation modes, so it was more dangerous. With v2, I wonder if we could re-consider this position?

Early return is fine. I tend to not use it because of FP influences, but avoiding stuff like this:

https://github.com/scikit-hep/awkward-1.0/blob/6638fc7d6ad082c4441598a803d93f0687ecfba1/src/awkward/_v2/operations/structure/ak_is_none.py#L57-L60

is worthwhile, I agree. On the other hand, for something like this:

https://github.com/scikit-hep/awkward-1.0/blob/6638fc7d6ad082c4441598a803d93f0687ecfba1/src/awkward/_v2/operations/structure/ak_is_none.py#L66-L69

I'd invert the predicate and only explicitly return the non-trivial case:

        if depth_context["posaxis"] == depth - 1:
            return layout.recursively_apply(getfunction_inner)

As you noticed, information for the next level is passed through depth_context and lateral_context, and the returned data is either the transformed Content or None. My intent with that was to make "action" functions be ignore the cases they don't care about (implicitly return None) instead of the pass_user stuff, where the user data might end up being None in such a case.

NB - I haven't enabled any tests that currently skip due to missing is_none - I noticed that one failed, and this is all the time I have to look at this for now.

Okay! Let me know when that changes and I'll review it.

[agoose77]

The broken test is caused by #1260, I believe

[agoose77]

@jpivarski I think this is also good to merge!

[jpivarski]

Indeed! Looks great!