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

refactor: move typetracer ufunc handling to backend [1 of 2] #2150

Merged
merged 6 commits into from
Jan 21, 2023

Conversation

agoose77
Copy link
Collaborator

@agoose77 agoose77 commented Jan 20, 2023

This PR splits the TypeTracer ufunc logic handling from #2127, using the Backend.apply_ufunc machinery to implement ufunc operations on ak.Arrays backed by `TypeTracer.

It implements "coercion" from UnknownScalar, UnknownLength, and MaybeNone values to the appropriate NumPy empty arrays, before applying the ufunc.

@agoose77 agoose77 changed the title refactor: move ufunc handling to backend refactor: move typetracer ufunc handling to backend Jan 20, 2023
@codecov
Copy link

codecov bot commented Jan 20, 2023

Codecov Report

Merging #2150 (03ef002) into main (f4bdf35) will increase coverage by 0.17%.
The diff coverage is 93.10%.

Additional details and impacted files
Impacted Files Coverage Δ
src/awkward/_backends.py 86.66% <92.30%> (+1.06%) ⬆️
src/awkward/_typetracer.py 72.41% <92.30%> (+5.07%) ⬆️
src/awkward/_connect/numpy.py 92.99% <100.00%> (-0.54%) ⬇️

elif x is ak._typetracer.UnknownLength:
return numpy.empty((0,), dtype=np.int64)
elif isinstance(x, ak._typetracer.MaybeNone):
return self._coerce_ufunc_argument(x.content)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@jpivarski this is one part I'm hesitant on. We're throwing away type information here because this might not fail at runtime, i.e. if the value is not actually none.

Copy link
Collaborator Author

@agoose77 agoose77 Jan 21, 2023

Choose a reason for hiding this comment

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

If we do want this, should MaybeNone also support operators (unrelated to this PR, but a more general point), e.g.. MaybeNone(x) + y? I'm inclined to say yes - MaybeNone should be fairly transparent, and serve as the outermost type like Haskell's Maybe.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

And, what should we do about OneOf? The proper approach would be to try all branches and re-wrap the result, but if one branch fails, we would need to handle that.

Copy link
Member

Choose a reason for hiding this comment

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

Our strategy is for typetracers to give code the benefit of the doubt and let it fail at runtime. The typetracer is not a type-check.

So if a typetracer (including MaybeNone or OneOf) represents $x$ or $y$ and $f(x)$ would yield some result but $f(y)$ would raise an error—great! We say that the result is $f(x)$.

If a typetracer represents $x$ or $y$ and both $f(x)$ and $f(y)$ would yield results, and they're different, that's a harder situation to deal with. I hope we don't have to because there might not be a good solution. We're not prepared to propagate non-deterministic type information. (If we had to, maybe we could go with the information-losing process of finding a common superclass of both results, like np.numerical.)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

OK, that sounds like the best path to me, too. Let's go with the safe, minimal approach and see what happens. I haven't implemented OneOf here, but we can do that in another PR, as it's probably better to keep momentum of the changes that are already in the pipeline.

Comment on lines +24 to +27
@property
@abstractmethod
def name(self) -> str:
raise ak._errors.wrap_error(NotImplementedError)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Misc cleanup.

@agoose77 agoose77 temporarily deployed to docs-preview January 20, 2023 23:59 — with GitHub Actions Inactive
@agoose77 agoose77 changed the title refactor: move typetracer ufunc handling to backend refactor: move typetracer ufunc handling to backend [1 of 2] Jan 21, 2023
@agoose77 agoose77 temporarily deployed to docs-preview January 21, 2023 19:20 — 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.

This looks good. I like it. Let's merge it!

@agoose77 agoose77 merged commit 707ea55 into main Jan 21, 2023
@agoose77 agoose77 deleted the agoose77/refactor-typetracer-ufunc-backend branch January 21, 2023 22:03
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