Conversation
Automated Review URLs |
willow-ahrens
left a comment
willow-ahrens
left a comment
There was a problem hiding this comment.
some comments from the call
| The `slice` key, when present, is a list of tuples of integers describing the starting and ending index of each dimension. If the `i`th tuple is `(a, b)`, then the `i`th dimension of the output should contain indices starting at `a` and ending just before `b`. | ||
|
|
||
| The operations when present are to be applied in the order `split_dims`, `combine_dims`, `slice`, followed by the `transpose` key if present. | ||
|
|
There was a problem hiding this comment.
If combine_dims, slice, split_dims, or transpose are present, a storage_shape` must be specified, to describe the shape before transformation
|
|
||
| The `slice` key, when present, is a list of tuples of integers describing the starting and ending index of each dimension. If the `i`th tuple is `(a, b)`, then the `i`th dimension of the output should contain indices starting at `a` and ending just before `b`. | ||
|
|
||
| The operations when present are to be applied in the order `split_dims`, `combine_dims`, `slice`, followed by the `transpose` key if present. |
There was a problem hiding this comment.
Leave a note that points out that combine dims can transpose too actually.
|
|
||
| As an example, an `11` by `37` BCSR can be represented as: | ||
|
|
||
| ```json |
There was a problem hiding this comment.
need outer brace here, also need commas, also there's no tuples in json
|
|
||
| In this section, we introduce an optional specification to split and combine dimensions. | ||
|
|
||
| Note that dimensions may not be able to be split or combined evenly. For example, if our original matrix is of size `5` by `7`, there is no way to use `2` by `2` blocks to tile the matrix evenly. In this case, we can pad our original matrix, decompose it into a tensor, and declare that the final matrix is a window into the full `6` by `8` matrix we would represent. For this reason, we introduce slicing operations into the spec. |
There was a problem hiding this comment.
So this idea of padding matrices for different formats is a little complicated to me. The dimensions of a matrix should be somewhat of an invariant (whereas the number of stored values is more flexible in sparse world) for a sparse matrix representation. It seems like it would be a better thing to just say that to split or combine into certain sizes blocks, the original matrix dimensions should be divisible by that block shape. I don't like the idea of a matrix changing shape depending on what representation is chosen for it.
There was a problem hiding this comment.
So this idea of padding matrices for different formats is a little complicated to me.
We don't pad by default. It'll be more like defining the logical matrix as a window into the physical blocked matrix, so that matrices with dimensions not divisible by the block size can be represented.
There was a problem hiding this comment.
hmm, to me, block sizes that don't fit dimenions should not be supported, but maybe i am in a minority here ... for an implementer, it means needing kernels that could handle blocks slice through domain in any which way and i don't think that makes sense at all ...
hameerabbasi
left a comment
hameerabbasi
left a comment
There was a problem hiding this comment.
This mostly LGTM as-is; just needs a cleanup of the JSON representation.
| "combine_dims"=[(0, 2), (1, 3)], | ||
| "slice"=[(0, 11), (0, 37)] |
There was a problem hiding this comment.
Probably run through a JSON validator here.
3 participants
After several discussions in the meetings, here's my attempt at adding some reshaping operators to support things like BSR. I've opted not to use a generic list of operations, instead choosing a specific application order. I wish we could have used strides, but there's no easy way to determine if they are well-formed. Instead, I chose to do a split-dimensions then combine-dimensions approach, which ensures that the reshapes always corresponds to bundling or un-bundling underlying dimensions that might have been raveled into the format. I also added a slice keyword at the end to handle padding, and because people asked for slicing.