Replace deprecated Eigen nonZeros() call for most recent Eigen versions. #1494
Conversation
Removal of nonZeros() call which was deprecated in https://gitlab.com/libeigen/eigen/-/commit/08da52eb8537107e2853452bb13c369856d1f84a corresponding to a redundant function call to size(). Issue: COLMAP does not build on Eigen 3.4.90 Fix: Adjust nonZeros() call to be size(). https://gitlab.com/libeigen/eigen/-/issues/2382
|
Thanks for the fix, but the new logic is incorrect. The size of the column is always 3. If |
Compute the L1-norm of the vectors to check for non-zeros. If vector has no non-zero elements, the norm will be zero, else the norm will be greater than zero. Updated with clarification from COLMAP devs re: checking for nonZero elements following Eigen implementation deprecation (and clarifying that original nonZeros() always returned size()).
|
Thanks @ahojnnes ! Apologies, I should have been clearer: my initial fix was merely removing the redundant function call since as far as I can tell nonZeros() has always returned size() (i.e. nonZeros has always been misleading). I've taken this comment from the Eigen repo issue and included below which implies this:
Checking through the file history on Gitlab to some random old commits, this looks to be the case. As you say, the size of these in COLMAP is always 3, which I believe means that the condition was never satisfied and those lines of code might not have ever run. In any case: your proposed fix works, I've just used the inbuilt l1-norm to wrap it up together. |
|
Thanks! |
…ns. (colmap#1494) * Remove nonZeros() call from Eigen::DenseBase Removal of nonZeros() call which was deprecated in https://gitlab.com/libeigen/eigen/-/commit/08da52eb8537107e2853452bb13c369856d1f84a corresponding to a redundant function call to size(). Issue: COLMAP does not build on Eigen 3.4.90 Fix: Adjust nonZeros() call to be size(). https://gitlab.com/libeigen/eigen/-/issues/2382 * Compute l1-norm to check for non-zeros Compute the L1-norm of the vectors to check for non-zeros. If vector has no non-zero elements, the norm will be zero, else the norm will be greater than zero. Updated with clarification from COLMAP devs re: checking for nonZero elements following Eigen implementation deprecation (and clarifying that original nonZeros() always returned size()).
|
I have a compiler error when building with Eigen 3.4.90 here: colmap/src/colmap/estimators/covariance.cc Line 188 in 88ade9d Should I replace it with a call to |
The `nonZeros()` method is a member of Eigen's sparse types, but `D_dense` is a dense vector - in this case it always returns `D_dense.size()`, which is incorrect. See related discussion in #1494.
Hi team,
Small change proposed which allows COLMAP to be built from source on the most recent versions of Eigen.
Removal of nonZeros() call which was deprecated in https://gitlab.com/libeigen/eigen/-/commit/08da52eb8537107e2853452bb13c369856d1f84a corresponding to a redundant function call to size().
Issue: COLMAP does not build on Eigen 3.4.90 (and presumably sub-releases since above commit).
Fix: Adjust nonZeros() call to be size().
This is a somewhat interesting issue, because if COLMAP intends to check the number of non-zero elements, the original implementation seemingly did not do that. If the intention was indeed to check non-zero elements a custom check of the matrices is needed pending an appropriate fix in Eigen.
More details on the Eigen front here:
https://gitlab.com/libeigen/eigen/-/issues/2382
Happy to discuss any other workarounds you can think of.
Cheers,
Jack