-
Notifications
You must be signed in to change notification settings - Fork 2
Arrays of user types #7
base: main
Are you sure you want to change the base?
Conversation
inducer
left a comment
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.
Thanks! I think this one will require some thought...
dagrt/builtins_python.py
Outdated
| return np.vdot(a, b) | ||
|
|
||
|
|
||
| def builtin_cumulative_product(a, axis): |
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.
Dagrt arrays don't have axes.
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 was only being used in one place in BDF, and I managed to remove it (562b2b5)
dagrt/builtins_python.py
Outdated
| def builtin_array_utype(n, x): | ||
| import numpy as np | ||
| if n != np.floor(n): | ||
| raise ValueError("array() argument n is not an integer") | ||
| n = int(n) | ||
|
|
||
| return np.empty((n, x.size), dtype=x.dtype) | ||
| #return np.zeros((n, x.size), dtype=x.dtype) |
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.
Could you explain the use case?
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 builtin is needed for when we need to index into a set of user types at runtime (i.e. we cannot create a set of distinct usertype variables at generation time). In the context of BDF/this PR, the use case is Nordsieck history arrays (see this paper, eq. 2.6), which in the context of variable order must (at least I think) be a single array of user types.
dagrt/function_registry.py
Outdated
| identifier = "<builtin>norm_2" | ||
|
|
||
|
|
||
| class NormWRMS(_NormBase): |
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.
Can you add these to the docs? (i.e. make it so that they show up in the documentation) There's a list in the module docstring.
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.
Done by 562b2b5.
dagrt/builtins_python.py
Outdated
| return np.vdot(a, b) | ||
|
|
||
|
|
||
| def builtin_cumulative_product(a, axis): |
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.
Could you explain the use case?
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 was only being used in one place in BDF, and I managed to remove it (562b2b5)
dagrt/function_registry.py
Outdated
| if check and not isinstance(x_kind, (NoneType, Array, UserType)): | ||
| raise TypeError("argument 'x' of 'array_utype' is not a user type") | ||
|
|
||
| return (Array(is_real_valued=True),) |
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 isn't true, really. Particularly the is_real_valued part.
dagrt/function_registry.py
Outdated
|
|
||
| is_real_valued = a_kind.is_real_valued and b_kind.is_real_valued | ||
|
|
||
| return (Array(is_real_valued),) |
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.
Also problematic.
dagrt/builtins_python.py
Outdated
| def builtin_reshape(a, a_cols): | ||
| return a.reshape(-1, a_cols, order="F") |
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.
Since dagrt arrays don't have shapes other than "1D vector", "reshape" is an odd addition IMO.
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 was only being used in one place in BDF, and I managed to remove it (562b2b5)
product builtins
for multiple UserTypes
builtin (avoids multi-UserType issue)
function end for temps
reusing user_type_move
moves, to avoid cluttering emit_assign_expr_inner
|
Could you remind me of the status of this? Is it ready to review? (If not, please mark as draft.) |
|
It will be ready soon, but isn't quite ready (need to put in place the outside-the-loop alloc/dealloc we talked about yesterday) |
|
@inducer this is ready for review - I'm removing the draft status after fixing what I could of the memory movement within loops. |
|
@inducer I've just looked this over again and brought it up to speed with the main If a refresher on motivation is needed, I would direct the reader to this paper, and in particular the use of Nordsieck history arrays for BDF integration as per this PR. |
This PR aims to add functionality for arrays of user types to
dagrt's builtins, for the purposes of generating code for the instructions provided by Leap in this PR.