Functions #364
Closed
Functions #364
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
inducer
commented
Oct 17, 2022
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
Merged
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
reviewed
Mar 13, 2023
kaushikcfd
force-pushed
the
functions
branch
2 times, most recently
from
50b0c68 to
10a909b
Compare
2 tasks
|
@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 <[email protected]>
Co-authored-by: Andreas Kloeckner <[email protected]>
inducer
commented
Apr 21, 2023
inducer
commented
May 17, 2023
| 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.
Suggested change
| return reduce(lambda a, b: a | b, args, frozenset()) | |
| return reduce(operator.or_, frozenset()) |
Comment on lines
+205
to
+207
| :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.
Suggested change
| :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".)
Comment on lines
+261
to
+263
| 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. |
This was referenced May 18, 2023
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
cc @kaushikcfd
This is a draft because: