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: don't call asarray on Index objects internally #2749

Merged
merged 1 commit into from
Oct 11, 2023

Conversation

agoose77
Copy link
Collaborator

@agoose77 agoose77 commented Oct 11, 2023

This PR follows #2740. I've updated that PR's description for context, but let me restart the problem here!

nplike.asarray ultimately invokes NumPy's array interface mechanisms when it acts upon an Index or Content object. If we don't make this PR change, an array with placeholders that hits this codepath will lead to an exception being thrown.

As outlined in #2740, we could update nplike.asarray to handle these types, but I'm really in favour of having our internals be strict where possible; both to do less work, but also to make it easier to reason about.

So, this PR replaces any as-yet uncaught uses of nplike.asarray(index) with index.data if the nplike is unchanged, or nplike.asarray(index.data) if not.

I haven't been exhaustive; anything that isn't being run in our test suite won't have been caught, but I'm happy enough with our coverage that those will hopefully not be too many that we can't deal with them as they come in.

I haven't added anything to the test suite to catch this. I'm slightly reluctant to e.g. add a strict-mode for our test suite that throws an exception if Index._array_interface_ is inspected. We could do this, though, and I'd welcome @jpivarski thoughts!

@agoose77
Copy link
Collaborator Author

agoose77 commented Oct 11, 2023

@jpivarski how do you feel about this PR?

Namely — considering nplike.asarray(index) an "error" internally, expecting it to fail under placeholders, but not adding any tests for this yet

Upfront it means we'll likely see exceptions that we could avoid by being more permissive in nplike.asarray, but I hope that this is just a bathtub model curve, and we'll have cleaner internals down the road.

@agoose77 agoose77 force-pushed the agoose77/fix-asarray-on-index branch from d88b2d0 to 8ecc0c9 Compare October 11, 2023 08:46
@agoose77 agoose77 force-pushed the agoose77/fix-asarray-on-index branch from 8ecc0c9 to cb8c70b Compare October 11, 2023 08:48
@codecov
Copy link

codecov bot commented Oct 11, 2023

Codecov Report

Merging #2749 (cb8c70b) into main (a6e426e) will increase coverage by 0.00%.
Report is 1 commits behind head on main.
The diff coverage is 78.78%.

Additional details and impacted files
Files Coverage Δ
src/awkward/contents/indexedarray.py 78.99% <100.00%> (ø)
src/awkward/contents/indexedoptionarray.py 88.48% <100.00%> (ø)
src/awkward/contents/unmaskedarray.py 74.40% <100.00%> (ø)
src/awkward/operations/ak_flatten.py 93.47% <100.00%> (ø)
src/awkward/operations/ak_run_lengths.py 90.12% <100.00%> (ø)
src/awkward/operations/ak_to_dataframe.py 90.55% <100.00%> (-0.08%) ⬇️
src/awkward/operations/ak_to_categorical.py 93.54% <50.00%> (ø)
src/awkward/operations/ak_unflatten.py 96.15% <75.00%> (ø)
...rc/awkward/operations/ak_merge_union_of_records.py 87.03% <68.75%> (+1.45%) ⬆️

... and 1 file with indirect coverage changes

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.

To make our tests more robust against the possibility of accidentally introducing dependence on Index.__array__ in the future, without forbidding it among end-users, we could add a private (L3) global setting that makes Index.__array__ an error when that global setting is turned on (default is off). The tests could then turn it on and be more strict than user code.

For this PR specifically, it all looks good.

@agoose77 agoose77 merged commit 2c69afd into main Oct 11, 2023
38 checks passed
@agoose77 agoose77 deleted the agoose77/fix-asarray-on-index branch October 11, 2023 16:29
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