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 constructor for OptionalJacobian #884

Merged
merged 1 commit into from
Oct 4, 2021

Conversation

gchenfc
Copy link
Member

@gchenfc gchenfc commented Oct 4, 2021

The fixed-size version of OptionalJacobian doesn't have a constructor for pointers to dynamic matrices. This makes it cumbersome to deal with e.g.:

double myFunc(double x, gtsam::OptionalJacobian<6, 6> H = boost::none) { ... }

class MyFactor : public gtsam::NoiseModelFactor {

  ...

  Vector unwhitenedError(const Values& x,
        boost::optional<std::vector<Matrix>&> H = boost::none) const override {
    double y = x.at<double>(keys_[0]);
    // myFunc(y, H ? &(*H)[0] : nullptr); //< this doesn't compile

    return H ? myFunc(y, (*H)[0])
             : myFunc(y); //< this does compile but is not as nice
  }
};

Frank recommends using the former "modern" syntax but this isn't possible without adding the constructor overload:

OptionalJacobian(Eigen::MatrixXd* dynamic) {...}

For future readers who are less familiar, ternary operator must have same-types for the middle and last parameters, and since the last parameter is a null then the middle parameter (and the whole output-type) has to be a pointer type.

No breaking changes and nothing removed, just added a new constructor overload.

@varunagrawal
Copy link
Collaborator

I'm wondering if we can use boost::none instead of nullptr.

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

@gchenfc
Copy link
Member Author

gchenfc commented Oct 4, 2021

Ya, in this case I used nullptr to stay consistent with the rest of the file, but I guess I usually use nullptr for brevity - do you think there's enough advantage that I should use boost::none in the future?

@gchenfc gchenfc merged commit e08800c into develop Oct 4, 2021
@gchenfc gchenfc deleted the feature/OptionalJacobian/dynamic_pointer branch October 7, 2021 01:13
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