Add matrix accessor format function to config and remove matrix_is_1d param #190
Conversation
hmartiro
force-pushed
the
codegen/1d_matrices
branch
from
48f137c to
68af77d
Compare
| # TODO(hayk): Fix this in follow-up. | ||
| # TODO(nathan): I don't think this deals with 2D matrices correctly | ||
| if config.matrix_is_1d and config.use_eigen_types: | ||
| if config.use_eigen_types: |
There was a problem hiding this comment.
For my own notes mostly, since it took me a bit to think through all this: the above changes don't affect matrices in LCM types, this change here affects only matrices in LCM types. So the above versions assume the objects are the language-native matrix types, for which we want to use format_matrix_accessor. Here we assume it's an LCM-type-internal matrix, which are represented by their storage. This primarily assumes you don't try to call a symforce-generated function with an LCM type containing a language-native matrix type, which is possible to do e.g. in Python where you can put a numpy.ndarray into an LCM type where we're expecting an eigen_lcm.Vector4d or whatever. This wasn't well supported before either though and seems fine to just not support at all, the one thing we might need to do is message that properly
bradley-solliday-skydio
force-pushed
the
codegen/1d_matrices
branch
from
68af77d to
3e7bd48
Compare
| if config.matrix_is_1d: | ||
| # TODO(nathan): Not sure this works for 2D matrices. Get rid of this. | ||
| formatted_symbols = [sf.Symbol(f"{key}[{j}]") for j in range(storage_dim)] | ||
| else: | ||
| # NOTE(brad): The order of the symbols must match the storage order of sf.Matrix | ||
| # (as returned by sf.Matrix.to_storage). Hence, if there storage order were | ||
| # changed to, say, row major, the below for loops would have to be swapped to | ||
| # reflect that. | ||
| formatted_symbols = [] | ||
| for j in range(value.shape[1]): | ||
| for i in range(value.shape[0]): | ||
| formatted_symbols.append(sf.Symbol(f"{key}({i}, {j})")) | ||
| # NOTE(brad): The order of the symbols must match the storage order of sf.Matrix | ||
| # (as returned by sf.Matrix.to_storage). Hence, if there storage order were | ||
| # changed to, say, row major, the below for loops would have to be swapped to | ||
| # reflect that. | ||
| formatted_symbols = [] | ||
| for j in range(value.shape[1]): | ||
| for i in range(value.shape[0]): | ||
| formatted_symbols.append( | ||
| sf.Symbol(config.format_matrix_accessor(key, i, j, shape=value.shape)) | ||
| ) |
There was a problem hiding this comment.
I didn't like how language specific logic (whether we render column vectors with one index (like v[3]) or with two (like v(3, 0))) was leaked into this codegen logic, and wanted to pull that into the logic of config.format_matrix_accessor.
Also, if for some language config we wanted to render 2d matrices as 1 long array (so m[r*COLS + c] instead of m[r, c], we'd wouldn't have enough information to do so.
So, by providing the shape of the matrix, format_matrix_accessor would have the info required to format correctly (regardless of row or column major storage order, and whether we want to store with 1 index or 2).
I chose to make shape a keyword-only argument to reduce the change of confusing the shape with the indices. As a side bonus, if we were to change the accessor to allow more than 2 indices, it should be easier this way.
| def test_matrix_accessor_python(self) -> None: | ||
| """ | ||
| Tests that matrices and vectors can be indexed properly. | ||
| """ | ||
|
|
||
| def left_multiply(m: sf.M22, v: sf.V2) -> sf.V2: | ||
| return T.cast(sf.V2, m * v) | ||
|
|
||
| output_dir = self.make_output_dir("sf_test_matrix_accessor_python") | ||
| namespace = "test_accessor" | ||
| generated_files = codegen.Codegen.function( | ||
| func=left_multiply, config=codegen.PythonConfig() | ||
| ).generate_function(namespace=namespace, output_dir=output_dir) | ||
|
|
||
| pkg = codegen_util.load_generated_package(namespace, generated_files.function_dir) | ||
|
|
||
| # NOTE(brad): If matrix indexing is incorrect (for example, | ||
| # we use m[3] instead of m[1, 1], or v[1, 0] instead of v[1]), then the | ||
| # function will raise an exception. | ||
| m = np.random.random((2, 2)) | ||
| v = np.random.random(2) | ||
| pkg.left_multiply(m, v) | ||
|
|
There was a problem hiding this comment.
This test currently fails with error:
| @@ -72,3 +72,7 @@ def printer(self) -> CodePrinter: | |||
| @staticmethod | |||
| def format_data_accessor(prefix: str, index: int) -> str: | |||
| return f"{prefix}.Data()[{index}]" | |||
|
|
|||
| @staticmethod | |||
| def format_matrix_accessor(key: str, i: int, j: int, *, shape: T.Tuple[int, int]) -> str: | |||
There was a problem hiding this comment.
Don't think shape needs to be keyword-only if it's a different type from everything else, but fine either way
| if shape[1] == 1: | ||
| return f"{key}[{i}]" |
There was a problem hiding this comment.
Maybe assert that j is 0 here?
There was a problem hiding this comment.
Should we instead just always assert that 0 <= i < shape[0] and 0 <= j < shape[1]?
There was a problem hiding this comment.
Yeah that seems reasonable too
… param Also, add test that indexing works correctly in python. Fixes #203 Topic: codegen/1d_matrix
bradley-solliday-skydio
force-pushed
the
codegen/1d_matrices
branch
from
3e7bd48 to
d1b7ac3
Compare
These files are being regenerated because we were previously incorrectly indexing into lcm_types even when `config.use_eigen_types` was True.
|
Closed by 652eb97 |
Also, add test that indexing works correctly in python.
Fixes #203
Topic: codegen/1d_matrix