-
Notifications
You must be signed in to change notification settings - Fork 150
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add diagonal & isotropic sqrt_infos in geo factors #294
base: bradley.solliday/revup/main/geo_factors_use_skip_directory_nesting
Are you sure you want to change the base?
Conversation
|
50c4a3a
to
cf82042
Compare
Often times the square root information matrix is a diagonal or isotropic matrix. However, in our generated geo factors we always assume they are full dense matrices leading to many more operations than needed. This commit modifies `geo_factors_codegen.py` to create 3 variants for each geo factor, one which assumes `sqrt_info` is a square matrix (the existing behavior), one which assumes it is a diagonal matrix, and one which assumes it is an isotropic matrix. These variants will have the same base name, with nothing appended to the square variant, `Diagonal` appended to the diaognal variant, and `Isotropic` appended to the isotropic covariant. They cannot have the same name, as even though we only generate the factors in C++ which has function overloading, disambiguating the overloads is a pain when constructing `sym::Factor`s. The square version takes a square eigen matrix, the diagonal version a eigen vector whose entries are those of the diagonal of `sqrt_info`, and a single scalar for the isotropic version. The header that the user imports remains the same, except now rather than containing the implementation directly, it re-imports the headers for the 3 different versions (which are stored in sub-directories). Topic: sqrt_info_variants Relative: geo_factors_use_skip_directory_nesting
Topic: sqrt_info_variants
cf82042
to
8e775ee
Compare
return wrapper | ||
|
||
|
||
def is_not_fixed_size_square_matrix(type_t: T.Type) -> bool: |
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.
Something like this that's simple and only used in one place should just be inlined
if "sqrt_info" not in func.__annotations__: | ||
raise ValueError( | ||
"sqrt_info missing annotation. Either add one or explicitly pass in expected number of dimensions" | ||
) | ||
sqrt_info_type = func.__annotations__["sqrt_info"] |
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.
We should use inspect.signature
(or other methods on inspect
) where possible, e.g. here (I get that above it's probably not possible since we're setting annotations to new values)
Often times the square root information matrix is a diagonal or
isotropic matrix. However, in our generated geo factors we always assume
they are full dense matrices leading to many more operations than
needed.
This commit modifies
geo_factors_codegen.py
to create 3 variants foreach geo factor, one which assumes
sqrt_info
is a square matrix (theexisting behavior), one which assumes it is a diagonal matrix, and one
which assumes it is an isotropic matrix.
These variants will have the same base name, with nothing appended to
the square variant,
Diagonal
appended to the diaognal variant, andIsotropic
appended to the isotropic covariant. They cannot have thesame name, as even though we only generate the factors in C++ which has
function overloading, disambiguating the overloads is a pain when
constructing
sym::Factor
s. The square version takes a squareeigen matrix, the diagonal version a eigen vector whose entries are
those of the diagonal of
sqrt_info
, and a single scalar for theisotropic version.
The header that the user imports remains the same, except now rather
than containing the implementation directly, it re-imports the headers
for the 3 different versions (which are stored in sub-directories).
Topic: sqrt_info_variants
Relative: geo_factors_use_skip_directory_nesting