Functions #364
Functions #364
Conversation
kaushikcfd
force-pushed
the
functions
branch
3 times, most recently
from
7846ab7 to
3b226ae
Compare
kaushikcfd
force-pushed
the
functions
branch
2 times, most recently
from
e060c67 to
edb1b19
Compare
kaushikcfd
force-pushed
the
functions
branch
5 times, most recently
from
36ca60a to
7378490
Compare
kaushikcfd
force-pushed
the
functions
branch
12 times, most recently
from
66e173d to
88bed5c
Compare
|
This is ready! And can be reviewed after we merge #387. |
kaushikcfd
force-pushed
the
functions
branch
6 times, most recently
from
6a81131 to
4f2deaa
Compare
kaushikcfd
force-pushed
the
functions
branch
2 times, most recently
from
50b0c68 to
10a909b
Compare
|
@inducer: This is ready to land, IMO. |
kaushikcfd
force-pushed
the
functions
branch
2 times, most recently
from
c39d7d3 to
a0ae9c7
Compare
Co-authored-by: Andreas Kloeckner <andreask@illinois.edu>
Co-authored-by: Andreas Kloeckner <andreask@illinois.edu>
inducer
left a comment
inducer
left a comment
There was a problem hiding this comment.
I'm still missing transform.calls, but I figured I would submit this to see how you feel about these comments.
inducer
left a comment
inducer
left a comment
There was a problem hiding this comment.
Here are a few more review comments for transform.calls, which I've started on.
But it occurred to me that this is sufficiently unwieldy that it's probably best to split this into "everything else" and the transforms for merge.
| def combine(self, *args: FrozenSet[CallSiteLocation] | ||
| ) -> FrozenSet[CallSiteLocation]: | ||
| from functools import reduce | ||
| return reduce(lambda a, b: a | b, args, frozenset()) |
There was a problem hiding this comment.
| return reduce(lambda a, b: a | b, args, frozenset()) | |
| return reduce(operator.or_, frozenset()) |
| :class:`CallSiteLocation`\ s being built. Must be altered (by creating | ||
| a new instance of the mapper) before entering the function body of a | ||
| new :class:`~pytato.function.Call`. |
There was a problem hiding this comment.
| :class:`CallSiteLocation`\ s being built. Must be altered (by creating | |
| a new instance of the mapper) before entering the function body of a | |
| new :class:`~pytato.function.Call`. | |
| :class:`CallSiteLocation`\ s being built. |
(Only of interest to implementers of subclasses, quite apparent from code. Plus I disagree with the phrasing "altered".)
| self.replacement_map, # type: ignore[attr-defined] | ||
| self.current_stack + (expr,), # type: ignore[attr-defined] | ||
| self.stack_to_replace_on, # type: ignore[attr-defined] |
There was a problem hiding this comment.
Why not just declare the types in the class body? (That is, IIUC this correctly.)
| stack_to_replace_on: Tuple[Call, ...]) -> None: | ||
| self.replacement_map = replacement_map | ||
| self.current_stack = current_stack | ||
| self.stack_to_replace_on = stack_to_replace_on |
There was a problem hiding this comment.
It looks like this is done one stack at a time. Is that the most efficient approach? (vs. batching)
| return super().map_named_call_result(expr) | ||
|
|
||
|
|
||
| def _have_same_axis_length(arrays: Collection[Array], |
There was a problem hiding this comment.
What (if not an error) should this return if the collection is empty?
| for other_ary in arrays) | ||
|
|
||
|
|
||
| def _have_same_axis_length_except(arrays: Collection[Array], |
There was a problem hiding this comment.
What (if not an error) should this return if the collection is empty?
|
It looks like 12f7524 ("Add regressions pt.inline_calls") contains a bit more than the commit message lets on. |
cc @kaushikcfd
This is a draft because: