wave_geography interface update #293
Conversation
Replaces raw arrays of doubles with Eigen matrices/vectors. Also updates wave_geography_tests.
…raphy_interface_update
Before, the API reference was not properly generated when running `make doc`
Descriptions of the functions already existed, just needed to add the doxygen tags so that the functions show up in the docs.
leokoppel
left a comment
leokoppel
left a comment
There was a problem hiding this comment.
Your changes look great!
Just thought of some more improvements to the interface
CMakeLists.txt
Outdated
| # Documentation | ||
| SET(WAVE_SOURCE_DIR ${CMAKE_CURRENT_SOURCE_DIR}) | ||
| SET(WAVE_BINARY_DIR ${CMAKE_CURRENT_BINARY_DIR}) | ||
| SET(WAVE_MODULES utils geometry containers benchmark controls kinematics matching vision optimization gtsam geography) |
There was a problem hiding this comment.
Ah I see, it's a regression from a previous commit where I deleted the LIBWAVE_MODULES list.
Is it possible to have the WAVE_ADD_MODULE function add to this list instead of writing it out here?
| */ | ||
| void ecefPointFromLLH(const double llh[3], double ecef[3]); | ||
| template <typename DerivedA, typename DerivedB> | ||
| void ecefPointFromLLH(const Eigen::MatrixBase<DerivedA> &llh, |
There was a problem hiding this comment.
Return ecef instead
F.20: For "out" output values, prefer return values to output parameters
| for (int j_col = 0; j_col < 3; ++j_col) { | ||
| EXPECT_NEAR(expected_R_ENU_ECEF[i_row][j_col], | ||
| T_ENU_ECEF[i_row][j_col], | ||
| EXPECT_NEAR(expected_R_ENU_ECEF(i_row, j_col), |
There was a problem hiding this comment.
You can now use Eigen's isApprox instead of looping over each matrix
Use the MatricesNear test predicate e.g.
(actually maybe a MatricesNearPrec is needed, here
https://github.com/wavelab/libwave/blob/master/wave_utils/include/wave/wave_test.hpp#L28 )
| double R_ENU_ECEF_result[3][3] = {{0.0, 1.0, 0.0}, // | ||
| Eigen::Matrix3d R_ENU_ECEF_result; | ||
| R_ENU_ECEF_result << 0.0, 1.0, 0.0, -1.0, 0.0, 0.0, 0.0, 0.0, 1.0; | ||
| /*double R_ENU_ECEF_result[3][3] = {{0.0, 1.0, 0.0}, // |
There was a problem hiding this comment.
Remove commented out code
| Eigen::Matrix4d T_ecef_enu; | ||
| ecefFromENUTransformMatrix(datum, T_ecef_enu, datum_is_llh); | ||
|
|
||
| // Affine inverse: [R | t]^(-1) = [ R^T | - R^T * t] |
There was a problem hiding this comment.
Nice this is way shorter than before!
| double T_enu_ecef[4][4], | ||
| bool datum_is_llh = true); | ||
| template <typename DerivedA, typename DerivedB> | ||
| void ecefFromENUTransformMatrix(const Eigen::MatrixBase<DerivedA> &datum, |
There was a problem hiding this comment.
Hmm these function interfaces are (already were) really confusing. This may be a good chance to change the names as well..
Whenever I see ecefFromENUTransformMatrix it seems like the function takes an "ENU transform matrix" and returns an "ecef".
It is also confusing to have a parameter datum_is_llh change the meaning of another parameter.
Maybe two separate functions
ecefEnuTransformFromEcef
ecefEnuTransformFromLlh
where the second function does the if (datum_is_llh) code and calls the first
There was a problem hiding this comment.
Actually if the ecefPointFromLLH function is changed to return the point, there is no need for the second function (where datum_is_llh) at all. The user can just chain the calls, ecefEnuTransformFromEcef(ecefPointFromLLH(x))
wave_geographyfilesPre-Merge Checklist:
clang-format