-
Notifications
You must be signed in to change notification settings - Fork 16
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
Introduce ReductionOperation
class, accept 'initial' in reductions
#238
Conversation
0c4efb9
to
01f5f0f
Compare
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 almost looks great! Found some minor numpy compat issues, feel free to merge after that.
Thanks!
|
||
|
||
def prod(a: Array, axis: Optional[Union[int, Tuple[int]]] = None) -> Array: | ||
def prod(a: Array, axis: Optional[Union[int, Tuple[int]]] = None, | ||
initial: Any = 1) -> Array: |
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.
Should this be _NoValue
as well? Numpy does it, see https://numpy.org/doc/stable/reference/generated/numpy.prod.html#numpy.prod.
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.
I think this is OK. Numpy will return 1 for an empty prod
. I think for an immutable object like 1
it does not matter whether that's done via _NoValue
and then setting internally, or directly like this.
8c02222
to
0a348e1
Compare
0a348e1
to
acf72ae
Compare
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.
Found out some more places where the implementation disagrees Numpy, good to go after that.
acf72ae
to
b3e359c
Compare
This replaces the strings we used to use to identify reductions with something a bit more structured. The impulse for this came from inducer/arraycontext#129, which needs to supply
initial
to array reductions to match thenumpy
interface for empty min/max reductions. This only adds very minimal support for supplyinginitial
, in that it allows the neutral element to be passed (and obtaining that is what led to the structured reduction op types), and it better mimics numpy's behavior for empty reductions.cc @majosm