Clang 3.4 reports a lot of "implicit conversion" warnings when compiling the tests (e.g. http://manao.inria.fr/CDash/viewBuildError.php?type=1&buildid=14193). Many of those are due to conversions of index types mapping to int or long. I have fixed these type issues by changing index types to match the index types of the accessed objects wherever possible. If this was not possible I converted the implicit conversion into an explicit one and adding internal asserts to be able to catch real overflows on conversion. Here are short descriptions of the problems and my solution proposals: eigen/unsupported/test/../../Eigen/src/SparseCore/SparseDenseProduct.h:141:58: warning: implicit conversion loses integer precision: 'Index' (aka 'long') to 'typename TransposeImpl<Block<DynamicSparseMatrix<float, 0, int>, -1, -1, false>, Sparse>::Index' (aka 'int') [-Wshorten-64-to-32] typename Traits::_RhsNested::InnerIterator it(rhs, outer); ~~ ^~~~~ In SparseDenseOuterProduct if Index type of the Lhs is narrower than Index of Rhs, then coeff access on Rhs shows this warning. I saw no possibility to promote the Index type. outer must be converted explicitly in SparseDenseProduct.h. -- eigen/unsupported/test/NonLinearOptimization.cpp:249:23: warning: implicit conversion loses integer precision: 'Index' (aka 'long') to 'const int' [-Wshorten-64-to-32] const int n = x.size(); Fixed by using VectorXd::Index instead of int in NonLinearOptimization.cpp. -- eigen/Eigen/src/Core/DenseCoeffsBase.h:499:60: warning: implicit conversion loses integer precision: 'Index' (aka 'long') to 'Index' (aka 'int') [-Wshorten-64-to-32] derived().coeffRef(row, col) = other.derived().coeff(row, col); ~~~~~ ^~~ In class DenseCoeffsBase the methods copyPacket(), copyCoeff() might access the other objects with an wider Index type. Fixed this with explicit conversions. -- eigen/Eigen/src/SparseCore/SparseMatrix.h:1076:23: warning: implicit conversion loses integer precision: 'Index' (aka 'long') to 'Index' (aka 'int') [-Wshorten-64-to-32] SparseMatrix dest(other.rows(),other.cols()); ~~~~ ^~~~~~~~~~~~ In operator= of SparseMatrix the Index type of the source may be narrower than those of the destination matrix. Solved by explicit conversion. -- eigen/Eigen/src/Core/AssignEvaluator.h:426:112: warning: implicit conversion loses integer precision: 'Index' (aka 'long') to 'int' [-Wshorten-64-to-32] copy_using_evaluator_innervec_InnerUnrolling<Kernel, 0, DstXprType::InnerSizeAtCompileTime>::run(kernel, outer); ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ^~~~~ The run method takes an hard coded int as index. Solved by using Index type of Kernel for the function argument. -- eigen/Eigen/src/Core/CoreEvaluators.h:580:19: warning: implicit conversion loses integer precision: 'Index' (aka 'long') to 'int' [-Wshorten-64-to-32] m_rowStride(map.rowStride()), ~^~~~~~~~~~~~~~~ Class members where hard coded ints. Solved by converting to members of type Index. -- eigen/Eigen/src/Core/Diagonal.h:161:14: warning: implicit conversion loses integer precision: 'long' to 'int' [-Wshorten-64-to-32] return m_index.value(); ~~~~~~ ^~~~~~~~~~~~~~~ Diagonal::index() returned hard coded int type. Solved by converting return type to Index. Also added obviously forgotten inline keyword to member function. -- eigen/Eigen/src/IterativeLinearSolvers/IncompleteLUT.h:34:13: warning: implicit conversion loses integer precision: 'Index' (aka 'long') to 'int' [-Wshorten-64-to-32] Index n = row.size(); /* length of the vector */ ~ ^~~~~~~~~~ Function template QuickSplit receives Index typename from function argument. If this is an int it may not match to the Index type of the Vector function argument. Solved by removing the independent Index template parameter and using Index of vector type instead. -- eigen/Eigen/src/SparseLU/SparseLU_SupernodalMatrix.h:236:15: warning: implicit conversion loses integer precision: 'Index' (aka 'long') to 'Index' (aka 'int') [-Wshorten-64-to-32] Index n = X.rows(); ~ ^~~~~~~~ In MappedSuperNodalMatrix<Scalar,Index>::solveInPlace() the Index of the destination matrix may be wider than the class Index type. Solved by explicit conversion. -- eigen/Eigen/src/SparseLU/SparseLU.h:690:15: warning: implicit conversion loses integer precision: 'Index' (aka 'long') to 'Index' (aka 'int') [-Wshorten-64-to-32] Index n = X.rows(); ~ ^~~~~~~~ Same as for MappedSuperNodalMatrix. -- eigen/Eigen/src/SparseLU/SparseLU_panel_bmod.h:209:54: warning: implicit conversion loses integer precision: 'long' to 'int' [-Wshorten-64-to-32] if(segsize==1) LU_kernel_bmod<1>::run(segsize, dense_col, tempv, glu.lusup, luptr, lda, nrow, glu.lsub, lptr, no_zeros); ~~~~~~~~~~~~~~ ^~~~~~~ Hard coded int types used in method arguments. Solved by using Index type arguments. -- eigen/Eigen/src/misc/SparseSolve.h:39:38: warning: implicit conversion loses integer precision: 'Index' (aka 'long') to 'Index' (aka 'int') [-Wshorten-64-to-32] inline Index rows() const { return m_dec.cols(); } ~~~~~~ ^~~~~~~~~~~~ Class sparse_solve_retval_base defined Index as Index of Lhs. Fixed by using promoted Index of Lhs and Rhs. -- eigen/Eigen/src/SparseQR/SparseQR.h:595:38: warning: implicit conversion loses integer precision: 'Index' (aka 'long') to 'Index' (aka 'int') [-Wshorten-64-to-32] inline Index cols() const { return m_other.cols(); } ~~~~~~ ^~~~~~~~~~~~~~ The Index type of class SparseQR_QProduct may not correspond to those of member m_other. Fixed by explicit conversion. -- eigen/unsupported/test/../../Eigen/src/Core/ReturnByValue.h:63:58: warning: implicit conversion loses integer precision: 'Index' (aka 'long') to 'Index' (aka 'int') [-Wshorten-64-to-32] EIGEN_DEVICE_FUNC inline Index rows() const { return static_cast<const Derived*>(this)->rows(); } ~~~~~~ ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ The ReturnType of KroneckerProductSparse was defined with Index=int. Fixed by defining it to be equal to promoted index of Rhs::Index and Lhs::Index. -- eigen/unsupported/Eigen/src/KroneckerProduct/KroneckerTensorProduct.h:154:28: warning: implicit conversion loses integer precision: 'Index' (aka 'long') to 'Index' (aka 'int') [-Wshorten-64-to-32] dst.resize(this->rows(), this->cols()); ~~~ ^~~~~~~~~~~~ Mixing of Rhs::Index and Lhs:Index in various loop variables. -- eigen/unsupported/Eigen/src/LevenbergMarquardt/LMqrsolv.h:35:15: warning: implicit conversion loses integer precision: 'Index' (aka 'long') to 'int' [-Wshorten-64-to-32] Index n = s.cols(); ~ ^~~~~~~~ Mixing of types of permutation matrix index and other matrix index in template function void lmqrsolv(). Fixed compiler warning on implicit integer conversion by separating index type for matrix and permutation matric which may not be equal. -- I have prepared pull request #83 with the fixes for the default branch: https://bitbucket.org/eigen/eigen/pull-request/83 All changesets are independent from each other as they fix different warnings. Please have a look at it.
Thank you for looking at them and fixing them individually. I won't apply them to the default branch but in the evaluator branch because 1) some of them won't apply anymore (dead code), and 2) some are already fixed by deeper refactoring. This might take some time because the merge is not straightforward.
With Rev 6414 no more "implicit conversion" warnings are issued (clang 3.5 compiler). This bug can be closed and the pull request should be rejected.
Forget about my last comment. I saw too late that the warnings were not enabled. I will have a look at them and will propose some new fixes.
I prepared a new pull request which fixes of the easy to fix implicit integer conversion warnings. https://bitbucket.org/eigen/eigen/pull-request/85 There are still issues for which I do not have a solution proposal.
I applied all changes, and re-enabled the respective warning.
A small suggestion: It might be a good idea to hide Index conversions from bigger to smaller types into a function which also handles the assertions. At the moment we have a mixture of eigen_assert and eigen_internal_assert. template<typename IndexDest, typename IndexSrc> inline IndexDest convertIndex(const IndexSrc& idx) { eigen_assert(idx <= NumTraits<IndexDest>::highest() && "Index to big ..."); return IndexDest(idx); } Usage: StorageIndex idx = convertIndex<StorageIndex>(idx_); At the moment, we don't really have a lot of conversions, so it is not too important.
Very good idea. This will also permit to easily find then for future enhancements, debugging....
Another suggestion for the index conversion: The runtime asserts are only needed for conversions to narrower type. The following templates take care of this: template<typename IndexDest, typename IndexSrc, bool WithAssert = (sizeof(IndexDest)<sizeof(IndexSrc))> struct IndexConvert { static inline IndexDest run(const IndexSrc& idx) { return IndexDest(idx); } }; // template specialization for conversion to narrower type w/ runtime overflow assert check template<typename IndexDest, typename IndexSrc> struct IndexConvert<IndexDest, IndexSrc, true> { static inline IndexDest run(const IndexSrc& idx) { eigen_assert(idx <= NumTraits<IndexDest>::highest() && "Index value to big for target type"); return IndexDest(idx); } }; template<typename IndexDest, typename IndexSrc> inline IndexDest convertIndex(const IndexSrc& idx) { return IndexConvert<IndexDest, IndexSrc, (sizeof(IndexDest)<sizeof(IndexSrc))>::run(idx); }
I started fixing this, but got stuck with some decisions: 1) At the moment when operations involve different Index types (most common when mixing SparseMatrix<..., int> with DenseMatrix) the generated expression has promote_index_type<Idx1, Idx2>::type as Index, which is the bigger of the two. That results in lots of conversion warnings, mostly when Iterators are initialized. Wouldn't it make sense here, to use the _smaller_ of the index types as common index type and assert only at initialization that both sizes fit into that Index type? 2) It would simplify a lot of things, if PermutationMatrix would typedef its StorageIndexType as Index -- most importantly, when calculating products between PermutationMatrix and SparseMatrix. Maybe, for convenience, we could add a constructor which accepts arbitrary integer types and asserts that the value is smaller that NumTraits<Index>::highest(). An alternative could be to specialize the product-traits for PermutationMatrices, replacing PermutationType::Index by PermutationType::StorageIndexType. 3) Some pivoting decompositions use internal::plain_{col,row}_type<MatrixType, Index>::type instead of TranspositionsType to store transpositions, I guess that has just not been ported yet? Nevertheless, the transpositions should use the same type as the Permutation, and the probably the Index typedef of the decomposition should be the smallest of MatrixType::Index and PermutationType::Index. Or at least that should be used as the "working Index".
(In reply to Georg Drenkhahn from comment #8) > Another suggestion for the index conversion: The runtime asserts are only > needed for conversions to narrower type. [...] I guess any decent compiler is capable of optimizing this assertion away. So I would save the template construct, unless someone observes a regression without it. We might actually distinguish between assert and internal_asserts at some points, because some conversions should never fail, as their input data had been checked before -- on the other hand, in most cases this should better be caught by choosing the Index type properly.
(In reply to Christoph Hertzberg from comment #9) > 1) At the moment when operations involve different Index types (most common > when mixing SparseMatrix<..., int> with DenseMatrix) the generated > expression has promote_index_type<Idx1, Idx2>::type as Index, which is the > bigger of the two. That results in lots of conversion warnings, mostly when > Iterators are initialized. > Wouldn't it make sense here, to use the _smaller_ of the index types as > common index type and assert only at initialization that both sizes fit into > that Index type? For coefficient-wise operations mixing sparse and dense objects you are probably right, but when two sparse objects are involved, the sparse outcome is more likely to require more non-zeros than the inputs, and so so we should use the largest one. > 2) It would simplify a lot of things, if PermutationMatrix would typedef its > StorageIndexType as Index -- most importantly, when calculating products > between PermutationMatrix and SparseMatrix. Maybe, for convenience, we could > add a constructor which accepts arbitrary integer types and asserts that the > value is smaller that NumTraits<Index>::highest(). > An alternative could be to specialize the product-traits for > PermutationMatrices, replacing PermutationType::Index by > PermutationType::StorageIndexType. Then you'll get the same issues when mixing permutation and dense matrices. Actually, I'd rather change SparseMatrix to mimic PermutationMatrix wrt indices, that is, distinguish between the index type used for storage, and the "Index" type as used in dense Matrix (e.g., as iteration index in for loops, for rows(), cols(), row(Index), coeff(Index,Index), etc.). > 3) Some pivoting decompositions use > internal::plain_{col,row}_type<MatrixType, Index>::type instead of > TranspositionsType to store transpositions, I guess that has just not been > ported yet? Nevertheless, the transpositions should use the same type as the > Permutation, and the probably the Index typedef of the decomposition should > be the smallest of MatrixType::Index and PermutationType::Index. Or at least > that should be used as the "working Index". Indeed, they should rather use a Transpositions<int>, but DenseIndex should still be used for everything else as we observed a significant speedup when moving from int to std::ptrdiff_t.
(In reply to Gael Guennebaud from comment #11) > For coefficient-wise operations mixing sparse and dense objects you are > probably right, but when two sparse objects are involved, the sparse outcome > is more likely to require more non-zeros than the inputs, and so so we > should use the largest one. How about always using ptrdiff_t/size_t for nnz, independent of IndexType? But see 2). > > 2) It would simplify a lot of things, if PermutationMatrix would typedef its > > StorageIndexType as Index [...] > > Then you'll get the same issues when mixing permutation and dense matrices. > Actually, I'd rather change SparseMatrix to mimic PermutationMatrix wrt > indices, that is, distinguish between the index type used for storage, and > the "Index" type as used in dense Matrix (e.g., as iteration index in for > loops, for rows(), cols(), row(Index), coeff(Index,Index), etc.). You're absolutely right. It would actually be much simpler, if every function accepting Indexes would accept the same (ptrdiff_t) Index. We could save a lot of work determining the index type when accessing an element. And inside the accessed element, we only need to [internal_]assert (row<rows() && col<cols()) if required. Actually, then I don't see a purpose of determining the Index type of an expression anymore. Maybe except CwiseBinary<Sparse, Sparse>, where I still believe that, e.g. (SparseMatrix<..., int>() + SparseMatrix<..., long>()).eval() should always fit into a SparseMatrix<..., int>. > > 3) [...] probably the Index typedef of the decomposition should > > be the smallest of MatrixType::Index and PermutationType::Index. Or at least > > that should be used as the "working Index". > > Indeed, they should rather use a Transpositions<int>, but DenseIndex should > still be used for everything else as we observed a significant speedup when > moving from int to std::ptrdiff_t. Yes, true again. We only need to assert here once that max(rows(), cols()) < NumTraits<PermIndex>::highest() and for the rest, simply directly cast Index to PermIndex before inserting it into the transposition or permutation type.
(In reply to Christoph Hertzberg from comment #12) > > > 2) It would simplify a lot of things, if PermutationMatrix would typedef its > > > StorageIndexType as Index [...] > > > > Then you'll get the same issues when mixing permutation and dense matrices. > > Actually, I'd rather change SparseMatrix to mimic PermutationMatrix wrt > > indices, that is, distinguish between the index type used for storage, and > > the "Index" type as used in dense Matrix (e.g., as iteration index in for > > loops, for rows(), cols(), row(Index), coeff(Index,Index), etc.). > > You're absolutely right. It would actually be much simpler, if every > function accepting Indexes would accept the same (ptrdiff_t) Index. We could > save a lot of work determining the index type when accessing an element. And > inside the accessed element, we only need to [internal_]assert (row<rows() > && col<cols()) if required. > Actually, then I don't see a purpose of determining the Index type of an > expression anymore. Maybe except CwiseBinary<Sparse, Sparse>, where I still > believe that, e.g. > (SparseMatrix<..., int>() + SparseMatrix<..., long>()).eval() > should always fit into a SparseMatrix<..., int>. Recall that the index type passed to SparseMatrix is used to store the "inner indices" of the non zeros, but also the position of the first non-zero of each "inner vector". Consequently, we must ensure that highest(IndexType) is greater than nnz.
(In reply to Gael Guennebaud from comment #13) > Recall that the index type passed to SparseMatrix is used to store the > "inner indices" of the non zeros, but also the position of the first > non-zero of each "inner vector". Consequently, we must ensure that > highest(IndexType) is greater than nnz. Oh yes, true again. So for sparse compressed format we should indeed use the bigger type. Theoretically, it might make sense to distinguish between inner and outer index type, but that would require breaking the API. It is quite unfortunate how much memory is wasted, if nnz>2^31. Basically, all the higher bits in the row-indices are wasted, as well as most higher bits of the column-indices.
I think we generally agreed that the easiest/cleanest solution is to make all Index types in the interface equal ptrdiff_t (resp. EIGEN_DEFAULT_DENSE_INDEX_TYPE). The question is how to realize that (this concerns mostly the sparse API): A) Remove the local Index typedefs and only have a global Eigen::Index typedef. This breaks code for users who are using the current typedefs. B) Use Eigen::Index explicitly in the API. This requires a lot of search&replace C) Always typedef Eigen::ClassName::Index as Eigen::Index, for sparse objects add a typedef StorageIndex which equals the current Index (similar to, e.g., PermutationMatrix). This (silently) changes behavior for users who are using the current typedef. D) Other suggestions? C) would be easiest to implement, but I'm not too happy with that, e.g., in cases where someone uses something like SparseMatrix<?, ?, typename OtherSparse::Index>. I'm slightly tending to B) since it would have the lowest impact on user code.
Ideally A seems to be the cleanest and simplest option, but unfortunately this would severely break API compatibility. So I cannot think about any better option than B because it would be too weird that SparseMatrix<?,?,Foo>::Index be different than Foo (even though SparseMatrixBase<>::Index is not documented). My biggest concern about this option is the discrepancy it introduces wrt PermutationMatrix/Transpositions. Perhaps, PermutationMatrix should be changed to match the implementation strategy we'll take for SparseMatrix. It's not too late.
I made a pull request: https://bitbucket.org/eigen/eigen/pull-request/89/ Except for one minor issue, this should solve the problem. I simply used ::Index instead of Eigen::Index, I'm not sure if that makes a difference somewhere. If it does, it should be easy to change.
Cannot we find a better name for the globally defined Index type so that we reduce the ambiguities between the Index type used for storage and the API index type? The usage of the old Expr::Index and traits<Expr>::Index types would be deprecated as loop/API index types, and would be defined as the storage index type which for dense matrices is irrelevant and will remains an alias for ptrdiff_t. I know that implies a huge search and replace to clean Eigen's own implementation, but that seems to be worth the effort as the current situation is too confusing and error prone.
The required search&replace is annoying but manageable ... I would still like to return the actually stored index type (for methods like rows()) in order to not cause conversion warnings when returned types are stored in smaller types -- obviously innerIndexPtr(), etc have to return the stored type anyways. But we shall accept the API index type for every method taking an index. Basically two decisions have to be made: * Where shall we make dimension/conversion assertions? (Also internal or normal asserts?) Currently, many (or all?) .coeff() methods blindly dereference the internal arrays without asserting (not even internally) that the sizes suffice. If we keep that behavior, it will not make sense for conversion assertions (comment 6) here, either. Actually, hardly any conversion shall be needed anymore, except when really storing an index. * How shall we name the API index? Suggestions: * IndexAPI, APIIndex (a bit long, but better than Eigen::Index) * IIndex, IndexI (I for interface) * Idx, Indx (too short?) * Index_ (we used _ only for internal things so far, also likely will cause name-clashes) * ? And a minor question: Shall we also provide Eigen::Index as an alias, even though it won't be practically usable from inside Eigen most of the time? NB: The reason why the pull-request happened to work is because there is a using namespace Eigen; inside test/main.h. Maybe we should make some test files which ensure that including each (public) header individually does work (i.e. also no external dependencies are missing)
After some discussion on IRC, we decided to replace current occurrences of Index by StorageIndex everywhere and to introduce Eigen::Index as the global API-Index. DenseBase<>::Index will become an alias for Eigen::Index (practically, that is already the case) and may get removed in the future. At the moment I will not reintroduce SparseBase<>::Index typedefs, because they would interfere with the global Eigen::Index type, which is different from what, e.g., innerIdxPtr() returns. That way people using the (undocumented) SparseBase::Index type will get a (more or less) graceful error message.
New pull request: https://bitbucket.org/eigen/eigen/pull-request/92/
The pull request has been updated. In my opinion, it is good enough to be merged. To summarize, the changes are: - Introduce a global typedef Eigen::Index making Eigen::DenseIndex and AnyExpr<>::Index deprecated (default is std::ptrdiff_t). - Eigen::Index is used throughout the API to represent indices, offsets, and sizes. - Classes storing an array of indices uses the type StorageIndex to store them. This is a template parameter of the class. Default is int. - Methods that *explicitly* set or return an element of such an array take or return a StorageIndex type. In all other cases, the Index type is used.
(In reply to Gael Guennebaud from comment #22) > The pull request has been updated. In my opinion, it is good enough to be > merged. I can't spot any flaws, either. Assuming that you ran the unit-tests, you have my go-ahead to merge. If there are defects left, merging is also the best way to find them (and as it is only the devel-branch, that should be ok).
Merge done.
-- GitLab Migration Automatic Message -- This bug has been migrated to gitlab.com's GitLab instance and has been closed from further activity. You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.com/libeigen/eigen/issues/877.