Add generic container reductions#62
Conversation
alexfikl
force-pushed
the
reduce-array-container
branch
from
4cb52b5 to
d5e67e7
Compare
alexfikl
force-pushed
the
reduce-array-container
branch
from
d5e67e7 to
36b2a65
Compare
kaushikcfd
left a comment
kaushikcfd
left a comment
There was a problem hiding this comment.
Thanks! Left some high-level design choice questions.
| def _map_array_container_impl( | ||
| f: Callable[[Any], Any], | ||
| ary: ArrayContainerT, *, | ||
| reduce_func: Callable[[Any, Iterable[Tuple[Any, Any]]], Any] = None, |
There was a problem hiding this comment.
Honestly, not sure I'm loving this. (Writing this after reading much of the rest of this PR.) Why not make something specific to reductions instead of trying to generalize this by force? (via lots of inefficient function wrapping)
There was a problem hiding this comment.
@inducer The main reason I did it like this was because _multimap_array_container_impl has a lot more logic in there that I felt squimish about largely copy pasting for the reduction case. I can add an if in there to avoid the wrapping. What do you think?
There was a problem hiding this comment.
I see your point. How about this: Duplicate map, but leave the wild wrap-and-dispatch thing in place for multimap? I can't really think of a "pretty" way to deal with this.
|
Unsubscribing... @-mention or request review once it's ready for a look or needs attention. |
inducer
left a comment
inducer
left a comment
There was a problem hiding this comment.
One typo, otherwise LGTM. Thanks for working on this! @kaushikcfd?
kaushikcfd
left a comment
kaushikcfd
left a comment
There was a problem hiding this comment.
Thanks, this looks great to me! The same piece of logic was being re-used at multiple places and this provides a nice primitive for it.
Co-authored-by: Andreas Klöckner <inform@tiker.net>
|
🎉 In it goes. |
Thanks for taking a look! |
3 participants
This adds some helpers to do reductions over array containers.
rec_reduce_array_container(reduce_func, array_func, ary)function.actx.np.[sum, min, max].Fixes #61.