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: add return_value='simplified' to ak.transform and revamp ak.firsts/ak.singletons #1968

Merged
merged 8 commits into from
Dec 7, 2022

Conversation

jpivarski
Copy link
Member

@jpivarski jpivarski commented Dec 6, 2022

The first commit touches a lot of files because it propagates Content.simplified through recursively_apply, which was only partially done by #1939.

It also changes the ak.transform API to take a return_value = "none" | "original" | "simplified" argument instead of return_array = False | True. The ak.transform function hasn't gotten out into the wild (much) yet, so this is a chance to make that parameter an n-way enum, rather than grow into a bunch of booleans that are only sometimes meaningful (like return_simplified and return_array; the first means nothing if the second is False).

This should have been done earlier, but Content._recursively_apply is now a hidden implementation detail. The ak.transform API is the public one that we'll support. Having two ways of doing the same thing is opening the door to inconsistency.

All of that was a stepping-stone to revamping ak.firsts. I didn't change its behavior within testing scope, but implementing it in terms of Content._recursively_apply is more robust. Branching layouts should now work automatically; they were NotImplementedError before.

After revamping ak.singletons, I'll mark this ready for review.


📚 The documentation for this PR will be available at https://awkward-array.readthedocs.io/en/jpivarski-revamp-ak.firsts-and-ak.singletons/ once Read the Docs has finished building 🔨

@jpivarski jpivarski self-assigned this Dec 6, 2022
@jpivarski jpivarski added the pr-next-release Required for the next release label Dec 6, 2022
@codecov
Copy link

codecov bot commented Dec 6, 2022

Codecov Report

Merging #1968 (abb8396) into main (e5836e9) will increase coverage by 0.01%.
The diff coverage is 86.51%.

Additional details and impacted files
Impacted Files Coverage Δ
src/awkward/operations/ak_is_none.py 96.55% <ø> (ø)
src/awkward/record.py 80.51% <ø> (ø)
src/awkward/contents/numpyarray.py 90.44% <50.00%> (+0.01%) ⬆️
src/awkward/contents/bitmaskedarray.py 69.33% <75.00%> (-0.03%) ⬇️
src/awkward/contents/bytemaskedarray.py 88.46% <75.00%> (-0.17%) ⬇️
src/awkward/contents/indexedarray.py 77.56% <75.00%> (-0.09%) ⬇️
src/awkward/contents/indexedoptionarray.py 88.67% <75.00%> (-0.12%) ⬇️
src/awkward/contents/unionarray.py 83.41% <75.00%> (-0.09%) ⬇️
src/awkward/contents/unmaskedarray.py 70.88% <75.00%> (-0.06%) ⬇️
src/awkward/operations/ak_firsts.py 90.32% <86.95%> (+2.08%) ⬆️
... and 7 more

@jpivarski jpivarski marked this pull request as ready for review December 6, 2022 23:51
@jpivarski
Copy link
Member Author

The main thing that will look different to an ordinary user is that ak.singletons no longer descends to the first option-type level and works there; it needs an explicit axis argument. There were a few other functions that had to be upgraded that way once—I think that happened to ak.fill_none rather early—and ak.singletons is probably the last to need that correction. It's a user-facing API change.

ak.firsts already had an axis argument and it seemed to be working fine, apart from the fact that it didn't handle negative values.

So for ak.singletons and ak.firsts in particular, this PR served as a look-over to ensure that they're in good shape, plus the axis for ak.singletons. I had vaguely remembered these functions misbehaving for anything but the simplest jagged arrays, but that must have been fixed some time ago.

However, by reimplementing them with modern machinery (_recursively_apply), it reminded me about some things that needed to be fixed up in that machinery. I've been wanting to hide _recursively_apply for a while, and the issue with it needing to pass down a simplify option was just an oversight. Actually, a partial oversight, because UnionArray._recursively_apply was always applying the simplified constructor. It needs to be configurable, though.

So that's it from me: if you approve of these changes, let me know!

@jpivarski jpivarski linked an issue Dec 7, 2022 that may be closed by this pull request
Copy link
Collaborator

@agoose77 agoose77 left a comment

Choose a reason for hiding this comment

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

In general, I like this PR. I like the cleanups particularly.

I want to talk about the choice to move recursively_apply to a private method. I don't want this conversation to distract from the review, so I'll put this as a separate discussion

I'd be most happy if we split the recursively_apply rename into a separate PR to discuss separately. In that case, this PR gets a green light from me already.

However, I know that there is a time pressure here, so if you feel so strongly about #1969 and this particular part of the refactor, then it's your call - I do support merging this as-is.

src/awkward/operations/ak_firsts.py Show resolved Hide resolved
src/awkward/operations/ak_firsts.py Show resolved Hide resolved
@jpivarski jpivarski changed the title fix: hide _recursively_apply, add return_value='simplified' to ak.transform, and revamp ak.firsts/ak.singletons fix: add return_value='simplified' to ak.transform and revamp ak.firsts/ak.singletons Dec 7, 2022
@jpivarski
Copy link
Member Author

jpivarski commented Dec 7, 2022

@agoose77 Now this PR does not hide recursively_apply (that will be the next PR), but it does everything the title says it does.

@jpivarski jpivarski merged commit 2ef23b4 into main Dec 7, 2022
@jpivarski jpivarski deleted the jpivarski/revamp-ak.firsts-and-ak.singletons branch December 7, 2022 23:00
@jpivarski jpivarski removed the pr-next-release Required for the next release label Feb 15, 2023
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.

Check ak.firsts/ak.singletons semantics before the 2.0.0 release
2 participants