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: indexing into RegularArray with typetracer #2227

Merged
merged 5 commits into from
Feb 9, 2023

Conversation

agoose77
Copy link
Collaborator

@agoose77 agoose77 commented Feb 8, 2023

TL;DR

This PR

DR

This latter point (regularisation of indices) is infectious; if a length is not known, or the index is unknown, the result is unknown.

This PR is taking a step towards allowing int as indices alongside the scalar arrays belong to backend.index_nplike. We need this if we want concrete slices (array[2:4]) to preserve more information than unknown slices (array[x:y]).

@agoose77
Copy link
Collaborator Author

agoose77 commented Feb 8, 2023

@lgray would you mind confirming that this fixes your workflow?

@agoose77 agoose77 requested a review from jpivarski February 8, 2023 01:40
@lgray
Copy link
Contributor

lgray commented Feb 8, 2023

Yes this fixes the repro and the more complex actual use case. Thanks!

@codecov
Copy link

codecov bot commented Feb 8, 2023

Codecov Report

Merging #2227 (81bb5b8) into main (69df2f2) will decrease coverage by 0.02%.
The diff coverage is 68.96%.

Additional details and impacted files
Impacted Files Coverage Δ
src/awkward/_slicing.py 87.40% <0.00%> (ø)
src/awkward/_nplikes/array_module.py 89.84% <63.63%> (+0.49%) ⬆️
src/awkward/_nplikes/numpylike.py 74.81% <66.66%> (-0.10%) ⬇️
src/awkward/_nplikes/typetracer.py 74.92% <72.72%> (-0.04%) ⬇️
src/awkward/contents/regulararray.py 86.60% <100.00%> (-0.40%) ⬇️

@agoose77 agoose77 temporarily deployed to docs-preview February 8, 2023 02:07 — with GitHub Actions Inactive
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.

More loosening of assumptions about lengths is a good thing; just be careful about slices with negative step, as noted below.

Comment on lines 420 to 424
# Handle unknown sizes
if self._size is None:
nextsize = None
start = 0
step = 1 if head.step is None else head.step
Copy link
Member

Choose a reason for hiding this comment

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

If step is negative, should start be 0?

Slices are completely different when the step is negative; be sure there is a test case for it.

@agoose77 agoose77 marked this pull request as draft February 8, 2023 16:44
@agoose77 agoose77 force-pushed the agoose77/fix-regulararray-index branch from c50fb18 to 81bb5b8 Compare February 9, 2023 22:05
@agoose77 agoose77 marked this pull request as ready for review February 9, 2023 22:05
@agoose77 agoose77 requested a review from jpivarski February 9, 2023 22:06
@agoose77 agoose77 temporarily deployed to docs-preview February 9, 2023 22:19 — with GitHub Actions Inactive
@agoose77 agoose77 enabled auto-merge (squash) February 9, 2023 22:21
@agoose77 agoose77 merged commit 25955f4 into main Feb 9, 2023
@agoose77 agoose77 deleted the agoose77/fix-regulararray-index branch February 9, 2023 22:30
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.

Indexing operations on regular array typetracer fails
3 participants