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

Working Hybrid #1802

Merged
merged 47 commits into from
Sep 5, 2024
Merged

Working Hybrid #1802

merged 47 commits into from
Sep 5, 2024

Conversation

varunagrawal
Copy link
Collaborator

@varunagrawal varunagrawal commented Aug 20, 2024

  1. Updated GaussianMixtureFactor to store the Gaussian normalizers as detailed in my thesis.
  2. Added 2 unit tests DifferingMeans and DifferingCovariances which verify the mode selection.
  3. Normalization of logProbabilities using a scheme detailed in my thesis.
  4. Formatting and additional comments.

@varunagrawal varunagrawal requested a review from dellaert August 20, 2024 20:41
@varunagrawal varunagrawal self-assigned this Aug 20, 2024
@varunagrawal varunagrawal requested a review from dellaert August 21, 2024 12:31
Base automatically changed from gaussian-bayes-net-improvements to develop August 21, 2024 20:58
@dellaert
Copy link
Member

@varunagrawal let me know when the examples are in here and I should look again.

@dellaert
Copy link
Member

PS, avoid pushing each commit to save on CI cycles :-)

@varunagrawal varunagrawal requested a review from dellaert August 28, 2024 22:40
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.

95% there, awesome progress.

@@ -115,6 +118,156 @@ TEST(MixtureFactor, Dim) {
EXPECT_LONGS_EQUAL(1, mixtureFactor.dim());
}

/* ************************************************************************* */
// Test components with differing means
Copy link
Member

Choose a reason for hiding this comment

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

I need more explanation in the comment to understand these tests. Also, since in the end they are just regression tests, it’s unclear they assert correctness of anything :-/ if these examples are supposed to be equivalent to the HBN tests above, and you are using the same values as there to test the result, that is a different story. Unclear now, though.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I should remove these since they are beyond the scope of this PR.

Copy link
Member

@dellaert dellaert Sep 4, 2024

Choose a reason for hiding this comment

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

Did you end up removing them?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Just removed them.

@dellaert
Copy link
Member

seems you have a compile issue. Could you maybe only push once a review is needed? That will eliminate some green-house gases when CI is run for entire matrix.

/home/runner/work/gtsam/gtsam/gtsam/hybrid/HybridGaussianFactorGraph.cpp:554:76: error: use of undeclared identifier 'f'
    if (auto gaussianMixture = dynamic_pointer_cast<GaussianMixtureFactor>(f)) {

@dellaert
Copy link
Member

Assuming the not 50-50 motion model test is the last bit. Let me know by email when I should look again.

@varunagrawal
Copy link
Collaborator Author

Assuming the not 50-50 motion model test is the last bit. Let me know by email when I should look again.

Yup, working on that.

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.

You did it! Some more detail comments that you can resolve yourself if taken care of. Awesome!

@dellaert
Copy link
Member

dellaert commented Sep 5, 2024

CI passed, want to merge?

@varunagrawal varunagrawal merged commit 232fa02 into develop Sep 5, 2024
34 checks passed
@varunagrawal varunagrawal deleted the working-hybrid branch September 5, 2024 13:25
@varunagrawal varunagrawal restored the working-hybrid branch September 5, 2024 13:26
@varunagrawal varunagrawal deleted the working-hybrid branch September 5, 2024 13:27
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