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 ManifoldGaussian class for messages in belief propagation #121

Merged
merged 30 commits into from Apr 20, 2022

Conversation

joeaortiz
Copy link
Contributor

Motivation and Context

It would be useful to have an optional covariance / precision matrix as part of the Manifold class as Gaussian Belief Propagation involves sending Gaussian distributions over the Manifold variables. Currently I'm using a wrapper Gaussian class but it could be more widely useful to have covariance / precision matrix as an attribute of the Manifold class?

How Has This Been Tested

Types of changes

  • Docs change / refactoring / dependency upgrade
  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have completed my CLA (see CONTRIBUTING)
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Mar 16, 2022
@mhmukadam mhmukadam added the refactor Refactor library components label Mar 16, 2022
@joeaortiz
Copy link
Contributor Author

I assume that all Manifold instances passed in the mean Sequence have the same batch size, dtype and device.
I haven't included the get and set item functions as the class stores two internal properties: mean and precision.

@mhmukadam mhmukadam marked this pull request as ready for review April 7, 2022 23:23
Copy link
Member

@mhmukadam mhmukadam left a comment

Choose a reason for hiding this comment

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

Looks good so far! On to unit tests. Any other features you are considering adding still?

@joeaortiz
Copy link
Contributor Author

Looks good so far! On to unit tests. Any other features you are considering adding still?

I don't think so. I was thinking of adding the local and retract functions but generalised to gaussians on manifold variables, but they would be better placed in the Manifold class.

@mhmukadam
Copy link
Member

mhmukadam commented Apr 8, 2022

local and retract functions but generalised to gaussians on manifold variables

Okay probably add it in this PR. Manifold already has a local and retract, how will this Gaussian version get incorporated?

theseus/geometry/se3.py Outdated Show resolved Hide resolved
Copy link
Member

@mhmukadam mhmukadam left a comment

Choose a reason for hiding this comment

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

Looks ready once we decide whether to move the local/retract inside the class.

Copy link
Member

@mhmukadam mhmukadam left a comment

Choose a reason for hiding this comment

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

LGTM! Is the plan to keep local/retract outside of the class?

@mhmukadam mhmukadam changed the title Add optional covariance / precision matrix to Manifold class Add ManifoldGaussian class for messages in belief propagation Apr 14, 2022
@mhmukadam mhmukadam added enhancement New feature or request and removed refactor Refactor library components labels Apr 14, 2022
@joeaortiz
Copy link
Contributor Author

Should be ready to merge now.

Copy link
Contributor

@luisenp luisenp left a comment

Choose a reason for hiding this comment

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

Looks good! Left some comments/questions. I didn't check the math closely.

theseus/optimizer/manifold_gaussian.py Outdated Show resolved Hide resolved
theseus/optimizer/manifold_gaussian.py Outdated Show resolved Hide resolved
theseus/optimizer/tests/test_manifold_gaussian.py Outdated Show resolved Hide resolved
theseus/optimizer/manifold_gaussian.py Show resolved Hide resolved
Copy link
Member

@mhmukadam mhmukadam left a comment

Choose a reason for hiding this comment

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

Look good to merge, nice work!

Copy link
Contributor

@luisenp luisenp left a comment

Choose a reason for hiding this comment

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

LGTM!

@mhmukadam mhmukadam merged commit 2769876 into main Apr 20, 2022
@mhmukadam mhmukadam deleted the joe.add_manifold_covariance branch April 20, 2022 15:17
luisenp added a commit that referenced this pull request Apr 26, 2022
* Change unit tests to avoid making mypy a main requirement. (#168)

* Change unit tests to avoid making mypy a main requirement.

* Added back dev requirements to CI.

* Wording change.

* Update readme and contrib (#169)

* Change unit tests to avoid making mypy a main requirement.

* Added back dev requirements to CI.

* Removed thanks file.

* Add missing license header in test_urdf_model

* Add ManifoldGaussian class for messages in belief propagation (#121)

* Gaussian class to wrap Manifold class and lam matrix for inverse covariance

* reformatted

* restored original manifold file

* initial attempt at marginal class , need to handle batch dim

* added standard fns dtype, to, copy, update

* single to call in init

* renamed ManifoldGaussian

* setting precision in init with checks

* update function requires mean and precision

* fixed naming in init

* manifold gaussian tests

* retract and local gaussian fns

* check precision is a symmetric matrix

* moved retract and local gaussian to manifold_gaussian to avoid circular imports

* added ManifoldGaussian to inits

* minor edits

* fixed dtype error in se3 that appeared in unit tests

* add checks for local_gaussian

* tests for local and retract gaussian

* import from th.

* added local_gaussian retract_gaussian to init, minor fix

* minor fix on value error

* fixed copy bug and added comments

* random precision matrix in unit tests

* fix for random precision

* init precision with identity

* fixed typo

* More efficient implementation of forward kinematics (#175)

* Bump DRM version and adapt API

* Add small tolerences to tests to account for numerical errors

* Changing setup virtualenv command. (#178)

* Updated SDF object in collision cost functions whenever an aux var is updated (#177)

* Updated SDF object in collision cost functions whenever an aux var is updated.

* Changed gather_from_rows_cols to support matrix broadcasting.

Co-authored-by: Mustafa Mukadam <mukadam.mh@gmail.com>
Co-authored-by: Joe Ortiz <joeaortiz16@gmail.com>
Co-authored-by: Austin Wang <austinswang@gmail.com>
luisenp pushed a commit that referenced this pull request Apr 26, 2022
* Gaussian class to wrap Manifold class and lam matrix for inverse covariance

* reformatted

* restored original manifold file

* initial attempt at marginal class , need to handle batch dim

* added standard fns dtype, to, copy, update

* single to call in init

* renamed ManifoldGaussian

* setting precision in init with checks

* update function requires mean and precision

* fixed naming in init

* manifold gaussian tests

* retract and local gaussian fns

* check precision is a symmetric matrix

* moved retract and local gaussian to manifold_gaussian to avoid circular imports

* added ManifoldGaussian to inits

* minor edits

* fixed dtype error in se3 that appeared in unit tests

* add checks for local_gaussian

* tests for local and retract gaussian

* import from th.

* added local_gaussian retract_gaussian to init, minor fix

* minor fix on value error

* fixed copy bug and added comments

* random precision matrix in unit tests

* fix for random precision

* init precision with identity

* fixed typo
suddhu pushed a commit to suddhu/theseus that referenced this pull request Jan 21, 2023
…okresearch#121)

* Gaussian class to wrap Manifold class and lam matrix for inverse covariance

* reformatted

* restored original manifold file

* initial attempt at marginal class , need to handle batch dim

* added standard fns dtype, to, copy, update

* single to call in init

* renamed ManifoldGaussian

* setting precision in init with checks

* update function requires mean and precision

* fixed naming in init

* manifold gaussian tests

* retract and local gaussian fns

* check precision is a symmetric matrix

* moved retract and local gaussian to manifold_gaussian to avoid circular imports

* added ManifoldGaussian to inits

* minor edits

* fixed dtype error in se3 that appeared in unit tests

* add checks for local_gaussian

* tests for local and retract gaussian

* import from th.

* added local_gaussian retract_gaussian to init, minor fix

* minor fix on value error

* fixed copy bug and added comments

* random precision matrix in unit tests

* fix for random precision

* init precision with identity

* fixed typo
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants