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: extend TypeTracerArray with __eq__, __ne__, and __array_ufunc__. #2021

Conversation

jpivarski
Copy link
Member

dask-contrib/dask-awkward#133 (comment) happened because TypeTracerArray didn't have an __eq__ method. It was easy to add, along with __ne__.

In addressing this issue, I first added an __array_ufunc__ (NEP-13) because I thought that's what the issue was and I was surprised that it didn't have one. I'm still surprised it doesn't have one. We could implement all of the __eq__, __ne__, __gt__, __ge__, __lt__, __le__ methods in one swoop by making TypeTracerArray inherit from ak._connect.numpy.NDArrayOperatorsMixin.

What do you think, @agoose77? Actually, I've pretty much talked myself into it. The next commit will do that. It's git, everything's reversible, of course!

@agoose77
Copy link
Collaborator

agoose77 commented Dec 19, 2022

This is definitely needed - the question for me is whether I'll be done with #1849 before this is 'needed' (it's already needed, but that's hard to define!)

Ths likelihood is that I'll be replacing this code in the near future. If you think it's needed now, merge! (I'll review anyway)

@agoose77 agoose77 enabled auto-merge (squash) December 19, 2022 18:51
@agoose77 agoose77 disabled auto-merge December 19, 2022 18:51
@jpivarski jpivarski temporarily deployed to docs-preview December 19, 2022 18:54 — with GitHub Actions Inactive
@jpivarski
Copy link
Member Author

"Why not make TypeTracerArray inherit from NDArrayOperatorsMixin?"

  1. It would cause a circular import unless NDArrayOperatorsMixin is moved somewhere more central, such as ak._util.
  2. i = where.index(Ellipsis)
    starts failing because somewhere, len of a TypeTracerArray is now being called.
  3. Replacing that where.index with a loop over where to look for the Ellipsis results in yet more errors.

At step 3, I stopped looking into it. The refactor made the TypeTracerArray code a little nicer (dropped 6 very short methods), but not a lot, and it seems to be getting into some deep waters that I don't have time for right now. I.e. the cost-benefit is not good enough to keep following this line.

@jpivarski jpivarski self-assigned this Dec 19, 2022
@jpivarski jpivarski requested a review from agoose77 December 19, 2022 18:56
@jpivarski
Copy link
Member Author

Yes, it's needed in the short term, and it's fine if it gets refactored. The ak.where function triggers it, here:

tags = ak.index.Index8((npcondition == 0).view(np.int8))

((npcondition == 0) needs to return a TypeTracerArray, not a Python bool.) So I'll do a more useful thing and add a test of type-tracer through ak.where, so that when you refactor, it will not get lost.

@jpivarski jpivarski temporarily deployed to docs-preview December 19, 2022 19:13 — with GitHub Actions Inactive
@jpivarski
Copy link
Member Author

This is ready to merge (I see your informal assent, above). The test failures are unrelated, but they need to be fixed.

As soon as tests pass, we'll merge.

@jpivarski jpivarski enabled auto-merge (squash) December 19, 2022 19:24
@codecov
Copy link

codecov bot commented Dec 19, 2022

Codecov Report

Merging #2021 (59aed28) into main (9ab248c) will decrease coverage by 0.01%.
The diff coverage is 57.14%.

Additional details and impacted files
Impacted Files Coverage Δ
src/awkward/_typetracer.py 74.15% <50.00%> (-0.53%) ⬇️
src/awkward/operations/ak_to_numpy.py 100.00% <100.00%> (ø)

@jpivarski jpivarski temporarily deployed to docs-preview December 19, 2022 20:33 — with GitHub Actions Inactive
@jpivarski jpivarski disabled auto-merge December 19, 2022 20:38
@jpivarski jpivarski merged commit 2b85171 into main Dec 19, 2022
@jpivarski jpivarski deleted the jpivarski/extend-TypeTracerArray-to-additional-cases-used-in-codebase branch December 19, 2022 20:48
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