Skip to content

New docs: inference#2087

Merged
dellaert merged 24 commits intodevelopfrom
new-docs-inference
Apr 16, 2025
Merged

New docs: inference#2087
dellaert merged 24 commits intodevelopfrom
new-docs-inference

Conversation

@p-zach
Copy link
Copy Markdown
Member

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

New docs for inference

Workflow

The way I generated these (there is room for improvement/more efficiency, although considering the amount done, it was quick work):

  1. Upload inference folder compressed by repomix to xml to Gemini 2.5 Pro Preview 03-25; upload Pose2.ipynb from geometry; prompt: "I already have a notebook describing Pose2, see below. Using that notebook as an example, could you produce notebooks for the classes in inference? One notebook per class."
  2. Copy the output into separate .ipynb files.
  3. Run through each notebook and clean up the code, fixing any errors.
  4. There weren't enough formulas, so I asked "For any of the classes you generated documentation for, if there are relevant mathematical formulas, give them to me in markdown format (and tell me which class to put them in) and I can paste them into the notebooks." and copied those to the appropriate files.
  5. Prompt "Write an inference.md file in the same format as this nonlinear.md file which summarizes the functionalities in inference.", uploading nonlinear.md, and copy that output to inference.md.
  6. Edit myst.yml to include the new files.

Issues

There are several problems which are currently causing errors in the code cells:

  • No .roots() for any BayesTree implementation is exposed
  • No EliminationTree implementation is exposed
  • GaussianISAM doesn't have print or copy constructor exposed
  • Ordering.Colamd(variableIndex) seems to overwrite Ordering.Colamd(graph); you have to manually convert it like Ordering.Colamd(VariableIndex(graph))
  • LabeledSymbol(char, char, int) constructor is not exposed
  • Ordering.invert is not exposed
  • Receive error type object 'gtsam.gtsam.Ordering' has no attribute 'ColamdConstrainedLast' even though it seems like ColamdConstrainedLast is exposed. Not sure why.
  • VariableIndex[x] is not exposed

I will leave this as a draft PR until these problems are resolved.

@p-zach p-zach requested a review from dellaert April 10, 2025 14:51
@dellaert
Copy link
Copy Markdown
Member

@p-zach let me know when you have incorporated notes from our meeting

@p-zach
Copy link
Copy Markdown
Member Author

p-zach commented Apr 15, 2025

I have made all of the simple notebook changes. I will get the changes involving .i file modifications pushed once I finish them asap.

@p-zach
Copy link
Copy Markdown
Member Author

p-zach commented Apr 16, 2025

Resolved remaining issues:

  • Fixed errors in Key.ipynb, LabeledSymbol.ipynb by using ord() to convert Python strings to ints to match the C++'s expectations
  • Wrapped GaussianEliminationTree
  • The Ordering.Colamd VariableIndex overload was actually fine; I was misunderstanding the C++ templates.
  • Fixed VariableIndex
    • Moved operator[], empty to the .cpp
    • Added .at() which does bound checking, [] is now an unsafe access
  • Fixed issues in BayesTree, JunctionTree, Ordering, VariableIndex, EliminationTree, ISAM by expanding wrapper and clarifying

@p-zach p-zach marked this pull request as ready for review April 16, 2025 04:48
@dellaert
Copy link
Copy Markdown
Member

Awesome. The ord thing works but it’s a bit ugly. We resolved that for Symbol by have a gtsam.symbol function that takes a string. I forget where it’s defined. Maybe you could do the same, I.e., make a labeledSymbol function?

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.

Overall the quality of the docs is astoundingly good! I left some comments but it’s a bit hard to review on GitHub. We could look over the other notebooks in our meeting, that’s probably easier. The labeledSymbol and roles comments can be left for another PR.

Thanks for this excellent PR!

@p-zach
Copy link
Copy Markdown
Member Author

p-zach commented Apr 16, 2025

Modified according to comments (less ord & roles). Will merge after CI

@dellaert dellaert merged commit 3fa9da9 into develop Apr 16, 2025
30 of 36 checks passed
@dellaert dellaert deleted the new-docs-inference branch April 18, 2025 15:29
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