New user self-registration is currently disabled. Please email eigen-core-team @ lists.tuxfamily.org if you need an account.
Bug 1403 - JacobiSVD/BDCSVD cannot be used with scalars that does not support implicit type conversion.
JacobiSVD/BDCSVD cannot be used with scalars that does not support implicit t...
Status: RESOLVED FIXED
Product: Eigen
Classification: Unclassified
Component: Householder
3.3 (current stable)
All All
: Normal Unknown
Assigned To: Nobody
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2017-03-09 13:05 UTC by Shuntaro Yamazaki
Modified: 2017-06-09 13:52 UTC (History)
2 users (show)



Attachments
patch to JacobiSVD (811 bytes, patch)
2017-03-09 13:05 UTC, Shuntaro Yamazaki
no flags Details | Diff

Description Shuntaro Yamazaki 2017-03-09 13:05:05 UTC
Created attachment 781 [details]
patch to JacobiSVD

Build fails if JacobiSVD is instantiated with scalar types that does not support implicit conversion from int/float/double/etc. A typical example is the combination with ceres::Jet in ceres-solver (http://ceres-solver.org/).   

$ cat a.cpp
#include <Eigen/Dense>
#include <ceres/jet.h>
int main() {
  typedef Eigen::Matrix<ceres::Jet<double, 1>, Eigen::Dynamic, Eigen::Dynamic> Mat;
  Mat m(1, 1);
  Eigen::JacobiSVD<Mat> e(m);
  return 0;
}

$ clang++ -Ieigen -Iceres a.cpp
./Eigen/src/QR/ColPivHouseholderQR.h:556:41: error: invalid operands to binary expression ('Scalar' (aka 'ceres::Jet<double, 1>') and 'int')
      if (m_colNormsUpdated.coeffRef(j) != 0) {                 [<=======(1)]
          ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ^  ~
./Eigen/src/QR/ColPivHouseholderQR.h:559:25: error: incompatible operand types ('int' and 'RealScalar' (aka 'Jet<double, 1>'))
        temp = temp < 0 ? 0 : temp;                             [<=======(2)]
                        ^ ~   ~~~~
(more errors)

---------- * ---------- * ----------

Among the errors, the error like (1) can be solved by defining operator!=() as follows:

namespace ceres {
template <typename T, typename S, int N, std::enable_if_t<std::is_arithmetic<S>::value, std::nullptr_t> = nullptr>
bool operator!=(const Jet<T,N>& jet, const S& scalar) {
    return jet.a != scalar;
}
}                                                              [<=======(3)]

The error like (2), however, may not be solved without adding an implicit constructor to 'RealScalar' class; i.e. We need to modify the code in either eigen or ceres-solver.

I woule propose to fix this issue in Eigen. The rationale is as follows: Implicit type conversion is strictly prohibited in many projects including ceres-solver due to unexpected data loss and overlooked runtime cost. On the other hand, Eigen::AutoDiffScalar does not fully support dynamically-sized object (http://eigen.tuxfamily.org/bz/show_bug.cgi?id=1281) whereas ceres::Jet does. So replacing ceres::Jet with Eigen::AutoDiffScalar is not an option for now. Finally, nothing is bad about avoiding implicit type conversion.

Attached are the patch to JacobiSVD. Note that even after applying these patches, the compile of 'a.cpp' above will fail. To pass the build, we need to define many operators like (3), or apply more patches to the code in Eigen to completely eliminate implicit type conversions from scalar to RealScalar. The same issue exists in BDCSVD.
Comment 1 Gael Guennebaud 2017-06-09 12:46:28 UTC
Thank you for the report, fixed:

https://bitbucket.org/eigen/eigen/commits/dadabdfa31e3/ (devel)
https://bitbucket.org/eigen/eigen/commits/2ce63814ae99/ (3.3)
Comment 2 Gael Guennebaud 2017-06-09 13:52:54 UTC
More fixes for BDVSVD:

https://bitbucket.org/eigen/eigen/commits/3e1cd2f29ed0 (devel)
https://bitbucket.org/eigen/eigen/commits/e95fa43a6986 (3.3)

BTW, note that it actually not always a good idea to convert constants likes 0, 1, 2, ... to the target scalar type prior to arithmetic operations or comparison. For instance, for it is much faster to multiply an arbitrary precision floating point number by an integer than by a float, double or even worse another arbitrary precision floating point number. For auto-diff (say AD<double>), it is also faster to multiply or add one AD<double> by a double rather than operating on two AD<double>. This is why in 3.3 we introduced a NumTraits<>::Literal type to cast such literals as the main scalar type prefer.

Note You need to log in before you can comment on or make changes to this bug.