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

Adding logmap API which takes the object to the function argument #532

Merged
merged 2 commits into from
Sep 21, 2020

Conversation

ayushbaid
Copy link
Contributor

The LieGroup supports Logmap (log-map of the argument at identity) as well as logmap (logmap of the element between the current object and the argument).

The logmap variant is present in gtsam.i, and hence not available in the python wrapper.

This PR adds the logmap function where-ever Logmap was defined.

Testing details: I build the gtsam library and python wrapper, and checked Rot3. As the change is similar in every place, it should work for all cases.

Copy link
Collaborator

@ProfFan ProfFan left a comment

Choose a reason for hiding this comment

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

Awesome :)

@dellaert
Copy link
Member

Will merge as we discussed LieXXX stuff in meeting

@dellaert dellaert merged commit 2f62761 into develop Sep 21, 2020
@dellaert dellaert deleted the feature/logmap_wrap branch September 21, 2020 15:10
@varunagrawal
Copy link
Collaborator

Ummm this broke the Matlab wrapper.

I don't understand why logmap was added to LieMatrix and LieVector if they have been deprecated?
@ayushbaid

@dellaert
Copy link
Member

How did it break the matlab wrapper?
And: if we deprecate LieXXX, we should do it forcefully. I talked to @ProfFan about it

@varunagrawal
Copy link
Collaborator

Apparently the logmap functions are not defined in the LieMatrix and LieVector classes.

/home/varun/borglab/gtsam/build/gtsam/wrap/gtsam/gtsam_wrapper.cpp: In function ‘void gtsamLieVector_logmap_193(int, mxArray**, int, const mxArray**)’:
/home/varun/borglab/gtsam/build/gtsam/wrap/gtsam/gtsam_wrapper.cpp:4328:32: error: ‘struct gtsam::LieVector’ has no member named ‘logmap’; did you mean ‘Logmap’?
 4328 |   out[0] = wrap< Vector >(obj->logmap(p));
      |                                ^~~~~~
      |                                Logmap
/home/varun/borglab/gtsam/build/gtsam/wrap/gtsam/gtsam_wrapper.cpp: In function ‘void gtsamLieMatrix_logmap_212(int, mxArray**, int, const mxArray**)’:
/home/varun/borglab/gtsam/build/gtsam/wrap/gtsam/gtsam_wrapper.cpp:4492:32: error: ‘struct gtsam::LieMatrix’ has no member named ‘logmap’; did you mean ‘Logmap’?
 4492 |   out[0] = wrap< Vector >(obj->logmap(p));
      |                                ^~~~~~
      |                                Logmap

@ayushbaid
Copy link
Contributor Author

So should I remove this from LieMatrix and LieVector for the time being? Before we can figure out the deprecation possibility for these 2 classes?

@varunagrawal
Copy link
Collaborator

I made a PR since it was quick and I need this to work for another PR.

@ayushbaid can you please take a look at it? It should be quick :)

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.

None yet

4 participants