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: support typetracer in ak.unflatten #2293

Merged
merged 9 commits into from
Mar 9, 2023

Conversation

agoose77
Copy link
Collaborator

@agoose77 agoose77 commented Mar 8, 2023

This PR fixes assumptions of length and known values that dask-awkward's test-suite fails.

@agoose77 agoose77 requested a review from jpivarski March 8, 2023 21:58
@codecov
Copy link

codecov bot commented Mar 8, 2023

Codecov Report

Merging #2293 (18548cd) into main (7fc01d8) will increase coverage by 0.08%.
The diff coverage is 100.00%.

Additional details and impacted files
Impacted Files Coverage Δ
src/awkward/typing.py 88.88% <ø> (+5.55%) ⬆️
src/awkward/_util.py 75.60% <100.00%> (+0.59%) ⬆️
src/awkward/contents/listoffsetarray.py 80.65% <100.00%> (-0.06%) ⬇️
src/awkward/operations/ak_unflatten.py 95.83% <100.00%> (+0.31%) ⬆️
src/awkward/operations/ak_pad_none.py 100.00% <0.00%> (ø)
src/awkward/operations/ak_drop_none.py 100.00% <0.00%> (ø)
src/awkward/operations/ak_local_index.py 100.00% <0.00%> (ø)
...rc/awkward/operations/ak_merge_union_of_records.py 98.05% <0.00%> (+0.01%) ⬆️
src/awkward/operations/ak_concatenate.py 96.26% <0.00%> (+0.03%) ⬆️
src/awkward/operations/ak_cartesian.py 90.69% <0.00%> (+0.07%) ⬆️
... and 36 more

@agoose77 agoose77 temporarily deployed to docs-preview March 8, 2023 22:13 — with GitHub Actions Inactive
@agoose77 agoose77 temporarily deployed to docs-preview March 8, 2023 22:33 — 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.

We talked about this today, and I thought that if threading a typetracer through the whole function is difficult, it could be a candidate for a specialized path. But it looks like you did it!

I don't see any tests, though. (Tests for the specific case of ak.unflatten with typetracers.) Do they come up in dask-awkward?

Comment on lines -122 to +127
current_offsets = backend.index_nplike.empty(len(counts) + 1, dtype=np.int64)
current_offsets = backend.index_nplike.empty(counts.size + 1, dtype=np.int64)
Copy link
Member

Choose a reason for hiding this comment

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

So... counts was originally obtained via to_layout (layouts have .length, not .size), but then it was replaced by to_backend_array. I assume that means it could be NumPy, CuPy, a JAX DeviceArray, or a TypeTracerArray. All of these have a .size?

For a NumPy array, the .size is the product of shape, not its first element. Is there any way that the NumpyArray might be multidimensional? I suppose not, if it's a valid counts.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, a previous PR explicitly ensures that counts is a backend array, all of which have .size. We have an explicit check on counts.ndim to ensure that it's 1D!

Copy link
Collaborator Author

@agoose77 agoose77 Mar 8, 2023

Choose a reason for hiding this comment

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

I don't see any tests, though. (Tests for the specific case of ak.unflatten with typetracers.) Do they come up in dask-awkward?

Dask-awkward does test this, because it brought this to my attention from their test suite. However, we should really test it for ourselves (I'm being lazy 🤕). I'll add some tests before we merge.

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.

✔️ on tests/test_2293_unflatten_typetracer.py

@agoose77 agoose77 temporarily deployed to docs-preview March 8, 2023 23:54 — with GitHub Actions Inactive
@agoose77 agoose77 enabled auto-merge (squash) March 8, 2023 23:58
@agoose77 agoose77 disabled auto-merge March 8, 2023 23:59
@agoose77
Copy link
Collaborator Author

agoose77 commented Mar 9, 2023

Ah, I noticed a final corner-case: counts can be known for typetracer backends, and I'm trying to preserve that kind of information where possible. I modified the PR to normalise counts if its value is not known, rather than testing if the backend has known_data.

@agoose77 agoose77 enabled auto-merge (squash) March 9, 2023 00:03
@agoose77 agoose77 temporarily deployed to docs-preview March 9, 2023 00:17 — with GitHub Actions Inactive
@agoose77 agoose77 merged commit d66c32a into main Mar 9, 2023
@agoose77 agoose77 deleted the agoose77/fix-unflatten-typetracer branch March 9, 2023 00:20
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.

2 participants