Skip to content

Towards fixing Doxygen-to-docstring issues#2107

Merged
dellaert merged 3 commits intodevelopfrom
fix-docs-params-inherits
Apr 23, 2025
Merged

Towards fixing Doxygen-to-docstring issues#2107
dellaert merged 3 commits intodevelopfrom
fix-docs-params-inherits

Conversation

@p-zach
Copy link
Copy Markdown
Member

@p-zach p-zach commented Apr 23, 2025

Current issues

  1. Doxygen docs do not carry over to the Python docstring if parameter names don't match exactly, as discussed in Python docstring edge case #2104.
  2. Docs from a parent class's function do not carry over when child classes wrap the function. For example, expmap is wrapped under Pose2 (among others), but its docs are in LieGroup, so Pose2.expmap does not have a docstring.

Resolving issue 1

This PR changes many .i file functions to unify the names between the wrapper and the headers. I did this by writing a script (with Gemini) to automatically detect and fix issues of this type. I chose to change the .i files instead of anything in the .h because making changes in the .h naturally leads to a huge amount of errors in inline functions that need to be fixed manually (as well as changing the documentation comments, etc etc).

The script, however, is limited because C++ syntax is so complex; it's very difficult to write regexes etc. that can handle it, especially with cases like multi-line function declarations and templates. Because of this, the script (despite my best efforts) is somewhat unreliable and hard to use, so I will thus not add it to this repository.

I added documentation in DEVELOP.md that reflects the parameter name match requirement moving forward.

It's telling me I can't automatically merge, so I will make this a draft until I fix the conflict.

Resolving issue 2

This is a tough one. For LieGroup-derived classes (which I believe to be the most important class of this issue), I am not certain how to fix the issue. I don't know the best way to wrap LieGroup since it's based on a template, and my attempts to add the inherited functions like expmap have resulted in hard-to-decipher linker errors. I would welcome any feedback here.

For example, I tried adding this snippet to Pose2.h to be able to document these functions:

/// Applies exponential map to v and composes with *this
Pose2 expmap(const Vector3& v);
/// Applies logarithmic map to group element that takes *this to g
Vector3 logmap(const Pose2& p);

/// expmap with optional derivatives
Pose2 expmap(const Vector3& v, ChartJacobian H1, ChartJacobian H2 = {});

/// logmap with optional derivatives
Vector3 logmap(const Pose2& p, ChartJacobian H1, ChartJacobian H2 = {});

But compilation yields errors that look like this:
geometry.obj : error LNK2019: unresolved external symbol "__declspec(dllimport) public: class Eigen::Matrix<double,3,1,0,3,1> __cdecl gtsam::Pose2::logmap(class gtsam::Pose2 const &,class gtsam::OptionalJacobian<3,3>,class gtsam::OptionalJacobian<3,3>)" (__imp_?logmap@Pose2@gtsam@@QEAA?AV?$Matrix@N$02$00$0A@$02$00@Eigen@@AEBV12@V?$OptionalJacobian@$02$02@2@1@Z) referenced in function "public: __cdecl <lambda_a632a661d6bda3c684ae16a0bd4c2b9a>::operator()(class gtsam::Pose2 *,class gtsam::Pose2 const &,class Eigen::Ref<class Eigen::Matrix<double,-1,-1,0,-1,-1>,0,class Eigen::OuterStride<-1> >,class Eigen::Ref<class Eigen::Matrix<double,-1,-1,0,-1,-1>,0,class Eigen::OuterStride<-1> >)const " (??R<lambda_a632a661d6bda3c684ae16a0bd4c2b9a>@@QEBA@PEAVPose2@gtsam@@AEBV12@V?$Ref@V?$Matrix@N$0?0$0?0$0A@$0?0$0?0@Eigen@@$0A@V?$OuterStride@$0?0@2@@Eigen@@2@Z)

@p-zach
Copy link
Copy Markdown
Member Author

p-zach commented Apr 23, 2025

I did occasionally edit .h files when there were parameter names missing completely.

Also, since I always took the name from the .h and replaced the name in the .i, sometimes the wrapped parameter name becomes less clear. If desired, I can take the (probably long!) time to go in and choose the clearer parameter name in these cases, including changing the docs comments and inline implementations.

@p-zach p-zach requested review from dellaert and varunagrawal April 23, 2025 18:48
@p-zach p-zach marked this pull request as ready for review April 23, 2025 20:00
Copy link
Copy Markdown
Member

@dellaert dellaert left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!!!
We can talk about remaining issues in person.

@dellaert dellaert merged commit a80a923 into develop Apr 23, 2025
36 checks passed
@dellaert dellaert deleted the fix-docs-params-inherits branch April 23, 2025 22:48
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.

2 participants