-
Notifications
You must be signed in to change notification settings - Fork 768
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 definition of constraints #1789
base: develop
Are you sure you want to change the base?
Conversation
CI fails. Talking about CI, try to save on CO2 by not pushing every commit, but only after you think CI will pass. |
It seems pretty weird that only the Windows CI fails. Unfortunately I do not have a Windows environment to test this on. I was trying to tweak with different changes to see which causes the Windows CI to fail. |
*/ | ||
|
||
#include <gtsam/constraint/NonlinearInequalityConstraint.h> | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Include "
#include <gtsam/inference/FactorGraph-inst.h>"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added this line, CI still reports the same error in Windows environments.
@yetongatech error is due to a argument type not being recognized:
Try figuring out what type you want and somehow being explicit? I just looked at the error above but maybe you can dig deeper. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool ! Some comments/questions.
Still need to pass CI
@@ -6,6 +6,7 @@ project(gtsam LANGUAGES CXX) | |||
set (gtsam_subdirs | |||
base | |||
basis | |||
constraint |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this directory only contains constraint classes, this is ok. Otherwise (if optimization code will land here) could we rename this to constrained
?
double k_; | ||
|
||
public: | ||
SoftPlusFunction(const double k = 1) : Base(), k_(k) {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
doxygen block describing argument
double b_; | ||
|
||
public: | ||
SmoothRampPoly3(const double epsilon = 1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
doxygen block describing argument
double a_; | ||
|
||
public: | ||
SmoothRampPoly2(const double epsilon = 1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
doxygen block describing argument
* @brief Constructor. | ||
* | ||
* @param factor NoiseModel factor. | ||
* @param tolerance vector representing tolerance in each dimension. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
stale?
Vector unwhitenedError(const Values& x, OptionalMatrixVecType H = nullptr) const override { | ||
VALUE x1 = x.at<VALUE>(key()); | ||
if (H) { | ||
return evaluateError(x1, &(H->front())); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does the base class not have unwhitenedError? Why not? Would that not be easier?
@@ -60,14 +59,23 @@ struct BoundingConstraint1: public NoiseModelFactorN<VALUE> { | |||
virtual double value(const X& x, OptionalMatrixType H = | |||
OptionalNone) const = 0; | |||
|
|||
/** active when constraint *NOT* met */ | |||
bool active(const Values& c) const override { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is active not used? Or is it in base class? Backwards compat...
// note: still active at equality to avoid zigzagging | ||
double x = value(c.at<X>(this->key())); | ||
return (isGreaterThan_) ? x <= threshold_ : x >= threshold_; | ||
Vector unwhitenedExpr(const Values& x, OptionalMatrixVecType H = nullptr) const override { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same, base class unwhitenedError?
// note: still active at equality to avoid zigzagging | ||
double x = value(c.at<X1>(this->key1()), c.at<X2>(this->key2())); | ||
return (isGreaterThan_) ? x <= threshold_ : x >= threshold_; | ||
Vector unwhitenedExpr(const Values& x, OptionalMatrixVecType H = nullptr) const override { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same question about active
what is unwhitenedExpr
? Not documented
Vector evaluateError(const X1& x1, const X2& x2, | ||
OptionalMatrixType H1, OptionalMatrixType H2) const override { | ||
OptionalMatrixType H1 = nullptr, OptionalMatrixType H2 = nullptr) const { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You used {} elsewhere
I think if you rebase on current |
Seems still not working |
@yetongatech you might have to investigate a bit to get past windows issue. None of us have the answers exactly :-) |
First PR in the 4 PRs to implement nonlinear constrained optimization in GTSAM.
This PR
NonlinearEqualityConstraint
andNonlinearInequalityConstraint
inheriting from NoiseModelFactor to represent constraints.NonlinearEquality
to inherit fromNonlinearEqualityConstraint
, andBoundingConstraint
to inherit fromNonlinearInequalityConstraint
.