Check sparse matrix condition for Jacobian and Hessian constructors

symforce-org / symforce

Fast symbolic computation, code generation, and nonlinear optimization for robotics

https://symforce.org

Apache License 2.0

1.41k stars 145 forks source link

Check sparse matrix condition for Jacobian and Hessian constructors #293

Closed changh95 closed 1 year ago

changh95 commented 1 year ago

Hi, a keen user of symforce here!

Found some code that may lead to some confusion in the future. Suggesting a change for that.

Best, changh95

Variable jacobian_is_sparse should really be kIsSparseEigenType<T>() instead of !kIsEigenType<T>()
So is for hessian_is_sparse

changh95 commented 1 year ago

@aaron-skydio

Thanks for a prompt review! There's no build issues - the suggested code change is for readability change. I noticed this code as I was looking to make SymForce work in C++11 and replace Eigen usages with my custom container (e.g. vector / matrix).

Speaking of which, SymForce doesn't seem to necessarily have C++14 features (only some constexpr functions are being used for simplicity). Downgrading it to C++11 will allow more users to use SymForce. I'm in an engineering field that need to comply with MISRA C++11 safety guidelines, and popular nonlinear solvers like Ceres and GTSAM all require C++14.

aaron-skydio commented 1 year ago

Cool - yeah we haven't really tracked if there is anything specific we're doing that would be impossible in C++11 as opposed to C++14, but at minimum there are parts of C++14 that we use. We don't plan on adding C++11 support, but if you do get symforce to build on C++11 I'd be curious to see what's required