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 docstring generation for Python wrapper #2038

Open
wants to merge 6 commits into
base: develop
Choose a base branch
from
Open

Conversation

p-zach
Copy link
Member

@p-zach p-zach commented Mar 1, 2025

Adds the capability to extract Doxygen documentation from the C++ source code and insert these docs into the Python bindings. This means that the documentation is then available in the help() function. The functionality can be seen in these examples:

>>> import gtsam
>>> help(gtsam.Pose3.transformTo)
Help on instancemethod in module gtsam.gtsam:

transformTo(...)
    transformTo(*args, **kwargs)
    Overloaded function.

    1. transformTo(self: gtsam.gtsam.Pose3, point: numpy.ndarray[numpy.float64[3, 1]]) -> numpy.ndarray[numpy.float64[3, 1]]

    takes point in world coordinates and transforms it to Pose coordinates
    point: point in world coordinates
    Returns: point in Pose coordinates

    2. transformTo(self: gtsam.gtsam.Pose3, point: numpy.ndarray[numpy.float64[3, 1]], Hself: numpy.ndarray[numpy.float64[m, n], flags.writeable, flags.f_contiguous], Hpoint: numpy.ndarray[numpy.float64[m, n], flags.writeable, flags.f_contiguous]) -> numpy.ndarray[numpy.float64[3, 1]]

    takes point in world coordinates and transforms it to Pose coordinates
    point: point in world coordinates
    Hself: optional 3*6 Jacobian wrpt this pose
    Hpoint: optional 3*3 Jacobian wrpt point
    Returns: point in Pose coordinates

    3. transformTo(self: gtsam.gtsam.Pose3, points: numpy.ndarray[numpy.float64[m, n]]) -> numpy.ndarray[numpy.float64[m, n]]

    transform many points in world coordinates and transform to Pose.
    points: 3*N matrix in world coordinates
    Returns: points in Pose coordinates, as 3*N Matrix

>>> help(gtsam.Pose3.AdjointMap)
Help on instancemethod in module gtsam.gtsam:

AdjointMap(...)
    AdjointMap(self: gtsam.gtsam.Pose3) -> numpy.ndarray[numpy.float64[6, 6]]

    Calculate Adjoint map, transforming a twist in this pose's (i.e, body) frame to the world spatial frame Ad_pose is 6*6 matrix that when applied to twist xi $ [R_x,R_y,R_z,T_x,T_y,T_z] $, returns Ad_pose(xi) 

>>> help(gtsam.Cal3_S2.calibrate)
Help on instancemethod in module gtsam.gtsam:

calibrate(...)
    calibrate(*args, **kwargs)
    Overloaded function.

    1. calibrate(self: gtsam.gtsam.Cal3_S2, p: numpy.ndarray[numpy.float64[2, 1]]) -> numpy.ndarray[numpy.float64[2, 1]]

    Convert image coordinates uv to intrinsic coordinates xy.
    p: point in image coordinates
    Returns: point in intrinsic coordinates

    2. calibrate(self: gtsam.gtsam.Cal3_S2, p: numpy.ndarray[numpy.float64[2, 1]], Dcal: numpy.ndarray[numpy.float64[m, n], flags.writeable, flags.f_contiguous], Dp: numpy.ndarray[numpy.float64[m, n], flags.writeable, flags.f_contiguous]) -> numpy.ndarray[numpy.float64[2, 1]]

    Convert image coordinates uv to intrinsic coordinates xy.
    p: point in image coordinates
    Dcal: optional 2*5 Jacobian wrpt
    Dp: optional 2*2 Jacobian wrpt intrinsic coordinates
    Returns: point in intrinsic coordinates

To generate these Python docstrings, follow these steps:

  1. Build the Doxygen XML. It's better to use the built Doxyfile in build/<build-name>/doc, but it might still work if you use the doc/Doxyfile.in (if, for example, you haven't built GTSAM yet and don't want to build it just to compile the Doxyfile).
  2. Set the new Cmake variable GTSAM_BUILD_PYTHON_DOCS_FROM_XML to ON.
  3. Set the new Cmake path variable GTSAM_PYTHON_DOCS_XML_SOURCE to the path to the xml folder generated by Doxygen.
  4. Build GTSAM with Python and pip install . it.

The parser is fairly robust but not perfect. I encourage anybody to test it out on functions that have well-formatted documentation in the source code so that we can improve it (naturally, the parser can only work with what's in the source--bad or poorly formatted source docs --> bad or poorly formatted Python docs).

To build these docstrings automatically with the pypi package that @yambati03 is working on, the build process will need to be amended so that Doxygen XML is generated before GTSAM Python is built. It takes about 3 minutes to generate the Doxygen files on my machine, and that can probably be shortened if necessary by editing the Doxyfile to not build unnecessary files.

Lastly--this PR edits wrap. Does that mean the changes need to be duplicated in the wrap repo? Not sure what the practice is there.

@p-zach p-zach requested a review from dellaert March 1, 2025 16:31
Copy link
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.

Cool! Some issues:

  • indeed, wrap is pulled in from a different repo. Maybe @varunagrawal can comment on correct way to change wrap and get it back into GTSAM.
  • that also affects the flag names: wrap is not GTSAM, so cmake variables there have a different naming convention I think
  • Finally, is it possible to not need GTSAM_PYTHON_DOCS_XML_SOURCE?

@p-zach
Copy link
Member Author

p-zach commented Mar 5, 2025

Finally, is it possible to not need GTSAM_PYTHON_DOCS_XML_SOURCE?

Yes, it is now changed to look for xml in {project_root}/xml.

Regarding the other two points:

  1. The wrap repo states "For any code contributions to the wrap project, please make them on the wrap repository." Therefore I think the changes in this PR made within wrap/ should be reverted, applied to a PR in the wrap repo and merged, then wrap should be updated in GTSAM using e.g. update_wrap.sh.
  2. I wonder where to put the Cmake option to turn on Python docstring generation. PybindWrap.cmake? Or is it fine in HandleGeneralOptions.cmake?

I await @varunagrawal for confirmation on (1) and insight on (2).

@yambati03
Copy link
Contributor

The wrap repo states "For any code contributions to the wrap project, please make them on the wrap repository." Therefore I think the changes in this PR made within wrap/ should be reverted, applied to a PR in the wrap repo and merged, then wrap should be updated in GTSAM using e.g. update_wrap.sh.

This is pretty interesting. I wonder why this isn't just a Git submodule? Then, the wrap folder would be a link to the wrap repository and could be kept in sync via git submodule update. Out of curiosity, is there a reason it's done this way @varunagrawal?

@ProfFan
Copy link
Collaborator

ProfFan commented Mar 6, 2025

@yambati03 We opted to make wrap a subtree instead of a submodule. If you are interested you can read on the internet about the difference.
TLDR; GitHub tarballs won't work if you opt for a submodule.

@varunagrawal
Copy link
Collaborator

@p-zach check out https://github.com/borglab/wrap. You'll want to move your changes over there so that they don't get overwritten the next time we pull that subtree in.

Let me know if you need any help with understanding that repo. Happy to connect offline.

@varunagrawal
Copy link
Collaborator

@yambati03 We opted to make wrap a subtree instead of a submodule. If you are interested you can read on the internet about the difference. TLDR; GitHub tarballs won't work if you opt for a submodule.

I wouldn't say Github tarbulls don't work, but they need some extra work. Lots of people have gotten this to work with submodules.

@varunagrawal
Copy link
Collaborator

Also FYI, there are a lot of subfolders in wrap that were originally intended for documentation generation but I guess they were never completed? If @p-zach's updates work, we should nuke those others and clean up the repository as general housekeeping.

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.

5 participants