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

Added evaluation of expressions when inserting in Values #1577

Merged
merged 2 commits into from
Jul 14, 2023

Conversation

arutkowski
Copy link
Contributor

This PR removes the inconvenient need to evaluate expressions involving values of type Point2 or Point3 when passing them to Values::insert, as mentioned in #1489.

In other words, using Class Template Argument Deduction (CTAD) a user can now simply call insert(key, a + b) instead of insert(key, (a + b).eval()) or insert<Point2>(key, a + b).

The related functions Values::insert_or_assign and Values::update have been modified in a similar manner.

Unit tests have also been added to verify the functionality.


Details

The inconvenience was originally caused by the fact that Point2 and Point3 are really just Eigen::Vector2 and Eigen::Vector3, and since Eigen uses lazy evaluation, expressions involving vectors are not automatically evaluated to vectors until they are needed (which is why the use of the auto keyword is discouraged in many cases).

The solution that was implemented here was to add template specializations for values of type Eigen::CwiseUnaryOp and Eigen::CwiseBinaryOp. Both are needed so expressions can be chained together, as in 2*a + b - c.

Following the guidance for designing functions that take Eigen types as parameters, I tried to implement the following function prototype:

    template <typename Derived>
    void insert(Key j, const Eigen::MatrixBase<Derived>& val);

However, this produces a lot of errors related to invalid use of incomplete type struct Eigen::internal::traits<gtsam::Rot3>, which I don't currently understand.

Related Future Work

There is a related inconvenience involving the creation of factors, as in BetweenFactor(key1, key2, b - a, noise). It would be a bit more difficult to implement implicit evaluation for Eigen types of a and b since the base class of NoiseModelFactorN used for most factors only includes the noise model and not the actual measurement (b - a in this case). Thus, every factor would need to implement template specializations for Eigen::CwiseUnaryOp and Eigen::CwiseBinaryOp, or perhaps they could all inherit from something like a GenericFactor that would include both the noise model and the measurement.

Copy link
Collaborator

@varunagrawal varunagrawal left a comment

Choose a reason for hiding this comment

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

LGTM

@varunagrawal varunagrawal merged commit 4de2d46 into borglab:develop Jul 14, 2023
26 checks passed
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