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

Simplify geo_factors_codegen w/ skip_dir_nesting #295

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

bradley-solliday-skydio
Copy link
Collaborator

Previously, geo_factors_codegen.py was using an ad-hoc system of
generating all the C++ factors into a factors folder. It did this by
calling Codegen.generate_function to generate the code into a
temporary file, read the file into a string, then re-wrote the string
into the desired location. I assume this was to avoid all the fluff
that's generated by default with Codegen.generate_function.

However, there is the skip_directory_nesting optional argument for
Codegen.generate_function which does precisely that (I think the code
in this file might have been written before that option was added).

So, to reduce confusion (such as the confusion I faced when I first
started looking at this file) and complexity, I rewrote the code to
instead use the skip_directory_nesting argument.

Topic: geo_factors_use_skip_directory_nesting

Previously, `geo_factors_codegen.py` was using an ad-hoc system of
generating all the C++ factors into a `factors` folder. It did this by
calling `Codegen.generate_function` to generate the code into a
temporary file, read the file into a string, then re-wrote the string
into the desired location. I assume this was to avoid all the fluff
that's generated by default with `Codegen.generate_function`.

However, there is the `skip_directory_nesting` optional argument for
`Codegen.generate_function` which does precisely that (I think the code
in this file might have been written before that option was added).

So, to reduce confusion (such as the confusion I faced when I first
started looking at this file) and complexity, I rewrote the code to
instead use the `skip_directory_nesting` argument.

Topic: geo_factors_use_skip_directory_nesting
@bradley-solliday-skydio
Copy link
Collaborator Author

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

# head base diff date summary
0 f9a6f2d0 4c368815 diff Feb 1 10:39 AM 1 file changed, 13 insertions(+), 56 deletions(-)

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

1 participant