Skip to content
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

Open
wants to merge 2 commits into
base: bradley.solliday/revup/main/geo_factors_use_skip_directory_nesting
Choose a base branch
from

Conversation

bradley-solliday-skydio
Copy link
Collaborator

@bradley-solliday-skydio bradley-solliday-skydio commented Jan 21, 2023

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::Factors. 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

@bradley-solliday-skydio
Copy link
Collaborator Author

bradley-solliday-skydio commented Jan 21, 2023

Reviews in this chain:
#295 Simplify geo_factors_codegen w/ skip_dir_nesting
 └#294 Add diagonal & isotropic sqrt_infos in geo factors

@bradley-solliday-skydio
Copy link
Collaborator Author

bradley-solliday-skydio commented Jan 21, 2023

# head base diff date summary
0 50c4a3af f9a6f2d0 diff Jan 20 16:14 PM 57 files changed, 9704 insertions(+), 3915 deletions(-)
1 cf820425 f9a6f2d0 diff Jan 20 17:06 PM 43 files changed, 1 insertion(+), 1 deletion(-)
2 8e775eea f9a6f2d0 diff Feb 1 10:39 AM 57 files changed, 211 insertions(+), 196 deletions(-)

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
return wrapper


def is_not_fixed_size_square_matrix(type_t: T.Type) -> bool:
Copy link
Member

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

Comment on lines +141 to +145
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"]
Copy link
Member

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)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants