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 Discrete-Continuous factor graph machinery #973

Closed
wants to merge 458 commits into from

Conversation

varunagrawal
Copy link
Collaborator

This PR adds the following:

  1. DCFactor and DCFactorGraph code ported from DCSAM (@keevindoherty has already looked at this part).
  2. Split all non-templated code into .h and .cpp files.
  3. Move the expNorm function to DiscreteFactor.[h|cpp].
  4. Move the types in DCSAM_types.h i.e. DCValues and DCMarginals to their own files to conform to our codebase structure.
  5. Add a new test assertion assert_type which asserts whether an object has a specified type.
  6. Use of the assert_type for simple constructor tests for DCValues and DCMarginals.

@varunagrawal varunagrawal added the feature New proposed feature label Dec 20, 2021
@varunagrawal varunagrawal self-assigned this Dec 20, 2021
gtsam/hybrid/DCFactor.h Outdated Show resolved Hide resolved
* https://github.com/borglab/gtsam/blob/43e8f1e5aeaf11890262722c1e5e04a11dbf9d75/gtsam_unstable/discrete/AllDiff.cpp#L43
*
* @param continuousVals - an assignment to the continuous variables
* @param discreteVals -
Copy link
Member

Choose a reason for hiding this comment

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

What is the role for discreteVals here? @keevindoherty should provide this knowledge :-)

Copy link
Contributor

Choose a reason for hiding this comment

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

discreteVals is an assignment to the discrete variables. This is useful for a kind of discrete-continuous smart factor where we've eliminated some subset of variables conditioned on a joint discrete and continuous assignment. In our case, we are using these for semantic+geometric measurements with ambiguous associations in semantic SLAM. For max-product elimination, check out this max-mixture factor for example, or for sum-product, we have a discrete-continuous EMFactor here. There are perhaps other applications I haven't thought about yet. Not all DCFactors have to use these values, but we pass them in case they are needed.

gtsam/hybrid/DCFactor.h Outdated Show resolved Hide resolved
* -------------------------------------------------------------------------- */

/**
*
Copy link
Member

Choose a reason for hiding this comment

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

I don’t remember we had added this, @varunagrawal ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yup, I added this. This is point 4 in the PR comment: since DCValues and DCMarginals are structs in DC_types.h, I just moved them to their own file so as to be consistent with the rest of GTSAM.

I took a bit of design liberty here, so I hope that's okay?

Copy link
Member

Choose a reason for hiding this comment

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

Same comment about authorship...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This file has Kevin as the sole author as per your recommendation. I only added myself as author to testDCMarginals.cpp since I wrote that file from scatch.

DiscreteMarginals discrete;

DCMarginals dc_marginals(continuous, discrete);
EXPECT(assert_type("gtsam::DCMarginals", dc_marginals));
Copy link
Member

Choose a reason for hiding this comment

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

I don’t understand. Self-fulfilling prophecy?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is a barebones test for the default constructor. This ensures that everything is included correctly and that a basic operation works.

The idea is that since this is a test, it should assert something, so just assert that the type is as expected.

Copy link
Member

Choose a reason for hiding this comment

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

C++ is strongly typed. Test anything, but type is known. Let's cherry pick assert_type in a branch if we ever need it, but please leave out of this PR.

* -------------------------------------------------------------------------- */

/**
* @file testDCValues.cpp
Copy link
Member

Choose a reason for hiding this comment

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

Same: is this something you added Varun? Design review??

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, same reason as for DCMarginals, just an attempt to be consistent. Again, a bit of liberty taken.

Copy link
Member

Choose a reason for hiding this comment

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

OK, but please list Kevin as author if you did not change the functionality, just the location. If you did change any code, explain what it is.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Kevin is still the author on DCValues.h. I am the author on testDCValues.cpp since I wrote that whole file by myself (no copy involved).

Same situation for DCMarginals.h.

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.

Please convert to WIP - I'd like to play with mixture factor before we merge, to better understand.

@varunagrawal varunagrawal marked this pull request as draft January 5, 2022 14:12
@dellaert
Copy link
Member

I think we should close this in favor of @ProfFan 's PR, right? If you agree please close, otherwise let's talk over email...

@dellaert
Copy link
Member

Closing this again until Fan’s PR is merged and then we can re-open a substantially modified PR for this. @varunagrawal please don’t reopen without talking to me first about the pans regarding that.

@dellaert dellaert closed this May 21, 2022
@varunagrawal varunagrawal mentioned this pull request Jun 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New proposed feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants