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: unexpected behaviour of akwhere with arrays containing nones #3168

Conversation

tcawlfield
Copy link
Collaborator

There is a problem with ak.which (the three-argument version) where if both (1) a conditional array element is optional yet True/False, and (2) the array element NOT selected by the condition is optional and None, the resulting element will be None regardless of the value that ought to have been selected.

This still has failures and multiple TODOs in the code. But one group of unit tests passes!
@tcawlfield tcawlfield marked this pull request as ready for review July 17, 2024 03:26
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.

This is good! We talked about the refactoring of how broadcast_any_option_akwhere is dispatched, and I see that it has been updated.

The print statements, even though they're commented out, should be removed; just accept all the suggestions below.

Also, update this to main and re-run the tests. Then if everything passes, it can be merged.

Actually, regarding that: in our other PRs, the MacOS tests have been failing because GitHub removed MacOS 11 and the newer versions of MacOS have been compiling/linking code that can't be loaded. (Talk to @ianna if you have any ideas.) Is your PR really passing all of those tests? If it fails to pass after you've updated, maybe that's a hint as to what's going wrong: it might be subtly in one of the new GPU kernels, or something that was touched to add those GPU kernels (MacOS does not compile the GPU code itself, but it might have been affected in some unexpected way).

If, after updating to main, you see errors, then this PR will be approved for merging, but can't be merged until the MacOS errors are fixed. @ianna and I have talked, and we're considering temporarily turning off the tests that fail on MacOS so that PRs aren't blocked, but we can't make a release with that because it will break Uproot. (Uproot depends on the code that's failing to load.)

src/awkward/operations/ak_where.py Outdated Show resolved Hide resolved
src/awkward/operations/ak_where.py Outdated Show resolved Hide resolved
src/awkward/operations/ak_where.py Outdated Show resolved Hide resolved
src/awkward/operations/ak_where.py Outdated Show resolved Hide resolved
@tcawlfield tcawlfield merged commit a9e48eb into main Jul 19, 2024
40 checks passed
@tcawlfield tcawlfield deleted the 3098-unexpected-behaviour-of-akwhere-with-arrays-containing-nones branch July 19, 2024 16:39
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.

unexpected behaviour of ak.where with arrays containing Nones
3 participants