-
Notifications
You must be signed in to change notification settings - Fork 89
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
Conversation
Codecov Report
Additional details and impacted files
|
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) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
If a typetracer represents np.numerical
.)
There was a problem hiding this comment.
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.
@property | ||
@abstractmethod | ||
def name(self) -> str: | ||
raise ak._errors.wrap_error(NotImplementedError) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Misc cleanup.
This reverts commit f52401e.
There was a problem hiding this 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!
This PR splits the
TypeTracer
ufunc logic handling from #2127, using theBackend.apply_ufunc
machinery to implementufunc
operations onak.Array
s backed by `TypeTracer.It implements "coercion" from
UnknownScalar
,UnknownLength
, andMaybeNone
values to the appropriate NumPy empty arrays, before applying the ufunc.