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

ak.flatten and ak.ravel should test for nplike.ndarray, not np.ndarray. #1340

Merged

Conversation

jpivarski
Copy link
Member

No description provided.

@jpivarski jpivarski linked an issue Mar 3, 2022 that may be closed by this pull request
@jpivarski jpivarski enabled auto-merge (squash) March 3, 2022 20:07
@codecov
Copy link

codecov bot commented Mar 3, 2022

Codecov Report

Merging #1340 (6159651) into main (b2fd2be) will not change coverage.
The diff coverage is 100.00%.

Impacted Files Coverage Δ
src/awkward/_v2/operations/structure/ak_flatten.py 91.42% <100.00%> (ø)
src/awkward/_v2/operations/structure/ak_ravel.py 90.90% <100.00%> (ø)

@agoose77
Copy link
Collaborator

agoose77 commented Mar 3, 2022

@jpivarski Not sure if this is helpful, but I did a quick grep and found a few other places in v2 where we reference np.ndarray, e.g. ak.type. I didn't check whether they're actually resolving against numpy.ndarray, or if the usage is acceptable.

@jpivarski jpivarski disabled auto-merge March 3, 2022 20:20
@jpivarski
Copy link
Member Author

Do you mean these two?

https://github.com/scikit-hep/awkward-1.0/blob/61596516352066437e602040f0e89eb2d8a78a79/src/awkward/_v2/operations/structure/ak_fill_none.py#L54-L74

Actually, these are the fill_value for ak.fill_none, and they're coming from the user—NumPy arrays are one of the possible input types. The other cases I came across are similar.

Which cases are you talking about?

@jpivarski
Copy link
Member Author

ak.type is also just checking possible user inputs, which includes np.ndarray.

The thing that had to be fixed here is that these arrays are coming out of a layout (layout.completely_flatten(...)), and so they'd have whatever array type the nplike says. That's a different context from data that comes from a user.

@agoose77
Copy link
Collaborator

agoose77 commented Mar 3, 2022

Sure, I might be missing the obvious here. Is it possible that a user will knowingly call ak.XXX functions on TypeTracerArray? Is that a useful thing that a keen user might wish to do?

@jpivarski
Copy link
Member Author

It shouldn't be possible. The TypeTracer was created just to support Dask, though you may have found another good use for it in avoiding duplication between Content methods and Form methods.

It's in a module named with an underscore—there's no way to make one without invoking private code (so not even "mid-level," really "low-level"!). A user might get one from a Dask Awkward Array's meta, but hopefully this is considered private, too.

That doesn't preclude the possibility of accidents (bugs), but users aren't supposed to be able to see them.

@jpivarski
Copy link
Member Author

It looks like this is safe for me to merge (no more work to do).

@jpivarski jpivarski merged commit 62e15fd into main Mar 3, 2022
@jpivarski jpivarski deleted the jpivarski/flatten-and-ravel-should-test-for-nplike.ndarray branch March 3, 2022 21:24
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.

ak.flatten axis=None fails on typetracer array
2 participants