drake
drake copied to clipboard
RobotLocomotion
[SymbolicFormula] Make conversion to bool implicit (Eigen)
This is to allow building with the latest version of Eigen. Several functions eventually lead to return a < b or return a > b which does not trigger explicit conversion.
Example issue:
external/eigen/Eigen/src/Core/Visitor.h:232:77: error: no viable conversion from returned value of type 'drake::symbolic::Formula' to function return type 'bool'
static EIGEN_DEVICE_FUNC inline bool compare(Scalar a, Scalar b) { return a > b; }
^~~~~
external/eigen/Eigen/src/Core/Visitor.h:246:20: note: in instantiation of member function 'Eigen::internal::minmax_compare<drake::symbolic::Expression, 0, false>::compare' requested here
if(Comparator::compare(value, this->res)) {
^
external/eigen/Eigen/src/Core/Visitor.h:67:11: note: in instantiation of member function 'Eigen::internal::minmax_coeff_visitor<Eigen::CwiseUnaryOp<Eigen::internal::scalar_score_coeff_op<drake::symbolic::Expression>, const Eigen::Block<Eigen::Block<Eigen::Ref<Eigen::Matrix<drake::symbolic::Expression, -1, -1, 0>, 0, Eigen::OuterStride<-1>>, -1, 1, true>, -1, 1, false>>, false, 0>::operator()' requested here
visitor(mat.coeff(0, i), 0, i);
^
external/eigen/Eigen/src/Core/Visitor.h:195:101: note: in instantiation of member function 'Eigen::internal::visitor_impl<Eigen::internal::minmax_coeff_visitor<Eigen::CwiseUnaryOp<Eigen::internal::scalar_score_coeff_op<drake::symbolic::Expression>, const Eigen::Block<Eigen::Block<Eigen::Ref<Eigen::Matrix<drake::symbolic::Expression, -1, -1, 0>, 0, Eigen::OuterStride<-1>>, -1, 1, true>, -1, 1, false>>, false, 0>, Eigen::internal::visitor_evaluator<Eigen::CwiseUnaryOp<Eigen::internal::scalar_score_coeff_op<drake::symbolic::Expression>, const Eigen::Block<Eigen::Block<Eigen::Ref<Eigen::Matrix<drake::symbolic::Expression, -1, -1, 0>, 0, Eigen::OuterStride<-1>>, -1, 1, true>, -1, 1, false>>>, -1, false>::run' requested here
return internal::visitor_impl<Visitor, ThisEvaluator, unroll ? int(SizeAtCompileTime) : Dynamic>::run(thisEval, visitor);
^
external/eigen/Eigen/src/Core/Visitor.h:448:9: note: in instantiation of function template specialization 'Eigen::DenseBase<Eigen::CwiseUnaryOp<Eigen::internal::scalar_score_coeff_op<drake::symbolic::Expression>, const Eigen::Block<Eigen::Block<Eigen::Ref<Eigen::Matrix<drake::symbolic::Expression, -1, -1, 0>, 0, Eigen::OuterStride<-1>>, -1, 1, true>, -1, 1, false>>>::visit<Eigen::internal::minmax_coeff_visitor<Eigen::CwiseUnaryOp<Eigen::internal::scalar_score_coeff_op<drake::symbolic::Expression>, const Eigen::Block<Eigen::Block<Eigen::Ref<Eigen::Matrix<drake::symbolic::Expression, -1, -1, 0>, 0, Eigen::OuterStride<-1>>, -1, 1, true>, -1, 1, false>>, false, 0>>' requested here
this->visit(maxVisitor);
^
external/eigen/Eigen/src/Core/DenseBase.h:490:14: note: in instantiation of function template specialization 'Eigen::DenseBase<Eigen::CwiseUnaryOp<Eigen::internal::scalar_score_coeff_op<drake::symbolic::Expression>, const Eigen::Block<Eigen::Block<Eigen::Ref<Eigen::Matrix<drake::symbolic::Expression, -1, -1, 0>, 0, Eigen::OuterStride<-1>>, -1, 1, true>, -1, 1, false>>>::maxCoeff<0, long>' requested here
return maxCoeff<PropagateFast>(index);
^
external/eigen/Eigen/src/LU/PartialPivLU.h:376:55: note: (skipping 4 contexts in backtrace; use -ftemplate-backtrace-limit=0 to see all)
= lu.col(k).tail(rows-k).unaryExpr(Scoring()).maxCoeff(&row_of_biggest_in_col);
^
external/eigen/Eigen/src/LU/PartialPivLU.h:135:7: note: in instantiation of member function 'Eigen::PartialPivLU<Eigen::Matrix<drake::symbolic::Expression, -1, -1, 0>>::compute' requested here
compute();
^
external/eigen/Eigen/src/LU/PartialPivLU.h:314:3: note: in instantiation of function template specialization 'Eigen::PartialPivLU<Eigen::Matrix<drake::symbolic::Expression, -1, -1, 0>>::compute<Eigen::Matrix<drake::symbolic::Expression, -1, -1, 0>>' requested here
compute(matrix.derived());
^
external/linear_solve/drake/math/linear_solve.h:408:67: note: in instantiation of function template specialization 'Eigen::PartialPivLU<Eigen::Matrix<drake::symbolic::Expression, -1, -1, 0>>::PartialPivLU<Eigen::Matrix<drake::symbolic::Expression, -1, -1, 0>>' requested here
const internal::EigenLinearSolver<LinearSolverType, DerivedA> linear_solver(
^
external/drake/math/linear_solve.h:582:30: note: in instantiation of function template specialization 'drake::math::GetLinearSolver<Eigen::PartialPivLU, Eigen::Matrix<drake::symbolic::Expression, -1, -1, 0>>' requested here
: eigen_linear_solver_{GetLinearSolver<LinearSolverType>(A)} {
^
external/drake/multibody/plant/tamsi_solver.cc:718:65: note: in instantiation of member function 'drake::math::LinearSolver<Eigen::PartialPivLU, Eigen::Matrix<drake::symbolic::Expression, -1, -1, 0>>::LinearSolver' requested here
const math::LinearSolver<Eigen::PartialPivLU, MatrixX<T>> J_lu(J);
^
external/drake/common/symbolic/expression/formula.h:205:12: note: explicit conversion function is not a candidate
explicit operator bool() const { return Evaluate(); }
^
This one occurs in PartialPivLU. The same function or
external/eigen/Eigen/src/Core/Visitor.h:225:77: error: no viable conversion from returned value of type 'drake::symbolic::Formula' to function return type 'bool'
static EIGEN_DEVICE_FUNC inline bool compare(Scalar a, Scalar b) { return a < b; }
^~~~~
is eventually reached by ColPivHouseholderQR, JacobiSVD.
Since the auto-conversion was introduced for Eigen support to begin with, make it implicit to support broader Eigen scenarios.
This change is 
The problem with the show-cased error in the commit message is that it is called indirectly from Eigen, instead of directly like for example:
external/eigen/Eigen/src/Core/Visitor.h:225:77: error: no viable conversion from returned value of type 'drake::symbolic::Formula' to function return type 'bool'
static EIGEN_DEVICE_FUNC inline bool compare(Scalar a, Scalar b) { return a < b; }
^~~~~
external/eigen/Eigen/src/Core/Visitor.h:246:20: note: in instantiation of member function 'Eigen::internal::minmax_compare<drake::symbolic::Expression, 0, true>::compare' requested here
if(Comparator::compare(value, this->res)) {
^
external/eigen/Eigen/src/Core/Visitor.h:36:5: note: in instantiation of member function 'Eigen::internal::minmax_coeff_visitor<Eigen::CwiseUnaryOp<Eigen::internal::scalar_abs_op<drake::symbolic::Expression>, const Eigen::Matrix<drake::symbolic::Expression, 3, 1, 0>>, true, 0>::operator()' requested here
visitor(mat.coeff(row, col), row, col);
^
external/eigen/Eigen/src/Core/Visitor.h:195:101: note: in instantiation of member function 'Eigen::internal::visitor_impl<Eigen::internal::minmax_coeff_visitor<Eigen::CwiseUnaryOp<Eigen::internal::scalar_abs_op<drake::symbolic::Expression>, const Eigen::Matrix<drake::symbolic::Expression, 3, 1, 0>>, true, 0>, Eigen::internal::visitor_evaluator<Eigen::CwiseUnaryOp<Eigen::internal::scalar_abs_op<drake::symbolic::Expression>, const Eigen::Matrix<drake::symbolic::Expression, 3, 1, 0>>>, 3, false>::run' requested here
return internal::visitor_impl<Visitor, ThisEvaluator, unroll ? int(SizeAtCompileTime) : Dynamic>::run(thisEval, visitor);
^
external/eigen/Eigen/src/Core/Visitor.h:397:9: note: in instantiation of function template specialization 'Eigen::DenseBase<Eigen::CwiseUnaryOp<Eigen::internal::scalar_abs_op<drake::symbolic::Expression>, const Eigen::Matrix<drake::symbolic::Expression, 3, 1, 0>>>::visit<Eigen::internal::minmax_coeff_visitor<Eigen::CwiseUnaryOp<Eigen::internal::scalar_abs_op<drake::symbolic::Expression>, const Eigen::Matrix<drake::symbolic::Expression, 3, 1, 0>>, true, 0>>' requested here
this->visit(minVisitor);
^
external/eigen/Eigen/src/Core/DenseBase.h:485:14: note: in instantiation of function template specialization 'Eigen::DenseBase<Eigen::CwiseUnaryOp<Eigen::internal::scalar_abs_op<drake::symbolic::Expression>, const Eigen::Matrix<drake::symbolic::Expression, 3, 1, 0>>>::minCoeff<0, int>' requested here
return minCoeff<PropagateFast>(index);
^
external/drake/math/rotation_matrix.cc:139:18: note: in instantiation of function template specialization 'Eigen::DenseBase<Eigen::CwiseUnaryOp<Eigen::internal::scalar_abs_op<drake::symbolic::Expression>, const Eigen::Matrix<drake::symbolic::Expression, 3, 1, 0>>>::minCoeff<int>' requested here
u_A.cwiseAbs().minCoeff(&i); // uₘᵢₙ = u_A(i).
^
external/drake/common/symbolic/expression/formula.h:205:12: note: explicit conversion function is not a candidate
explicit operator bool() const { return Evaluate(); }
^
This one could be addressed on the Drake side by: https://github.com/jwnimmer-tri/drake/commit/97371d8d79174c0d415bda0ad2300d6c08f5d95b (brought up in https://github.com/RobotLocomotion/drake/pull/17590) but issues like the showcased cannot.
The two Eigen functions in question:
template<typename Scalar, int NaNPropagation, bool is_min=true>
struct minmax_compare {
typedef typename packet_traits<Scalar>::type Packet;
static EIGEN_DEVICE_FUNC inline bool compare(Scalar a, Scalar b) { return a < b; }
static EIGEN_DEVICE_FUNC inline Scalar predux(const Packet& p) { return predux_min<NaNPropagation>(p);}
};
template<typename Scalar, int NaNPropagation>
struct minmax_compare<Scalar, NaNPropagation, false> {
typedef typename packet_traits<Scalar>::type Packet;
static EIGEN_DEVICE_FUNC inline bool compare(Scalar a, Scalar b) { return a > b; }
static EIGEN_DEVICE_FUNC inline Scalar predux(const Packet& p) { return predux_max<NaNPropagation>(p);}
};
Allowing auto-conversion is not awesome. An alternative solution would be to template-specialize these types, BUT they are in Eigen::internal and it seems like a worse solution to interfere with that namespace.
This is to allow building with the latest version of Eigen.
What version, exactly?
For LDLT at least we have https://github.com/RobotLocomotion/drake/blob/master/common/symbolic/expression/ldlt.h which I know needs to be updated to play well with post-3.4.0 Eigen trunk, since Eigen's template spelling there have changed.
I have some other post-3.4.0 trunk patches under preparation at https://github.com/jwnimmer-tri/drake/commits/eigen-trunk-post-340 but I haven't prioritized landing much of it yet. My assumption was that because Eigen didn't seem anywhere near tagging a new 3.5 branch soon, it wasn't a priority to finish that work yet. Maybe you can shed some more light?
What version, exactly?
I am using: https://gitlab.com/libeigen/eigen/-/commit/9960a304222919b5ab419e2f66196fd4500c2af7 It is from 3 months ago - I will try with latest.
My assumption was that because Eigen didn't seem anywhere near tagging a new 3.5 branch soon, it wasn't a priority to finish that work yet. Maybe you can shed some more light?
I am not sure when 3.5 is coming out, but there were several breaking changes since 3.4 already and was looking to make the transition more incremental, as there are more coming.
Switched testing to latest Eigen trunk: https://gitlab.com/libeigen/eigen/-/commit/69f50e3a6732e854623695df273d0940db346ff5
A fix for the return a < b errors is still needed in that commit.
Understood, thanks. I'll look into this soon, but it's less of a code review and more of a thought-process review, so it'll take me somewhat longer than unusual to get to it.
Sorry for the delays, we prioritized C++20 and Xcode upgrades. Pre-release Eigen support is next on my list.