Bug 583 - Householder QR preconditioner asserts with unsigned index type
Householder QR preconditioner asserts with unsigned index type
Status: RESOLVED FIXED
Product: Eigen
Classification: Unclassified
Component: QR
3.2
All All
: Normal Unknown
Assigned To: Nobody
:
Depends on:
Blocks: 3.2
  Show dependency treegraph
 
Reported: 2013-04-11 09:43 UTC by Tim Besard
Modified: 2013-06-10 17:17 UTC (History)
2 users (show)



Attachments
Test-case to trigger the assertion. (410 bytes, text/x-c++src)
2013-04-11 09:43 UTC, Tim Besard
no flags Details
Compilation log. (9.89 KB, text/plain)
2013-04-11 09:44 UTC, Tim Besard
no flags Details
Backtrace from the triggered assertion. (4.09 KB, text/plain)
2013-04-11 09:44 UTC, Tim Besard
no flags Details

Description Tim Besard 2013-04-11 09:43:28 UTC
Created attachment 328 [details]
Test-case to trigger the assertion.

If I use a unsigned index type (#define EIGEN_DEFAULT_DENSE_INDEX_TYPE unsigned), all householder QR preconditioners trigger an assertion. Looking at the source it seems that the SVD code doesn't seem to be written with unsigned indexes in mind (for loops while --i > 0, etc).
Comment 1 Tim Besard 2013-04-11 09:44:37 UTC
Created attachment 329 [details]
Compilation log.

This compilation log, produces with Clang -Wall -Wextra, lists some of the code constructs which seem incapable of working with unsigned indices.
Comment 2 Tim Besard 2013-04-11 09:44:59 UTC
Created attachment 330 [details]
Backtrace from the triggered assertion.
Comment 3 Christoph Hertzberg 2013-04-18 14:32:49 UTC
Index is generally supposed to be a signed integer type (maybe this should be documented where EIGEN_DEFAULT_DENSE_INDEX_TYPE is described).
There may be many parts which also work with unsigned integers, but you can't rely on this always being the case.
Do you have a use case where you need that extra bit for the index type (and where going to the next bigger int-type is not an option)?
Comment 4 Tim Besard 2013-04-18 17:55:07 UTC
No, I just tried to use it since I prefer unsigned types for values which shouldn't go negative (such as matrix dimensions). Certainly no must-have, although it shouldn't fail as horribly as it did.
Comment 5 Christoph Hertzberg 2013-04-18 19:21:37 UTC
I guess changing that design decision is hardly possible now. However, it should be possible to add a (more descriptive) static assertion if the user provided index type is not signed.
Comment 6 Gael Guennebaud 2013-06-10 17:17:02 UTC
https://bitbucket.org/eigen/eigen/commits/a8c411cca7a3/
Changeset:   a8c411cca7a3
User:        ggael
Date:        2013-06-10 17:16:16
Summary:     Fix bug 583: add compile-time check that DenseIndex is signed

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