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

additional sparseJacobian functions (new) #677

Merged
merged 9 commits into from
Jan 19, 2021
Merged

Conversation

gchenfc
Copy link
Member

@gchenfc gchenfc commented Jan 19, 2021

This replaces #610 to reduce commit- and diff- noise.

The purpose of this PR is to provide a convenience function for obtaining the sparse (augmented) Jacobian of a GaussianFactorGraph.

Note that, as it stands, 99% of this PR is copy-pasta from GaussianFactorGraph::sparseJacobian(). Although I would prefer to make a version that is more elegant, this is the original way Mandy/Fan's code was written and I guess we can do that in a future PR. I added a TODO.

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.

Bit confused that your more general code is not here but I guess that will be next PR..

/// Constructs an Eigen-format SparseMatrix of a GaussianFactorGraph
SpMat sparseJacobianEigen(
const GaussianFactorGraph &gfg, const Ordering &ordering) {
// TODO(gerry): eliminate copy/pasta by making GaussianFactorGraph version
Copy link
Member

Choose a reason for hiding this comment

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

I thought you had done that already? Or is that "the next PR" ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Ya, I was under the impression that you wanted just the bare minimum thing needed for A->B, which is efficient code for returning an Eigen-sparse-matrix format jacobian?

Copy link
Member Author

Choose a reason for hiding this comment

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

But I'm more than happy to put that in this PR

Copy link
Member

Choose a reason for hiding this comment

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

No, my comment on the other PR was not that these more generic, copy/pasta avoiding functions were not useful, just that they should be inside gfg.h, and then used in a small header-only function for the Eigen version.

Copy link
Member

Choose a reason for hiding this comment

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

So, I propose you merge this, then create a next PR with those utilities to get rid of copy/pasta. No sense in throwing away that work.

Copy link
Member Author

Choose a reason for hiding this comment

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

ok!

gtsam/linear/SparseEigen.h Show resolved Hide resolved
gtsam/linear/tests/testGaussianFactorGraph.cpp Outdated Show resolved Hide resolved
@gchenfc gchenfc merged commit 8f13405 into develop Jan 19, 2021
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