New user self-registration is disabled due to spam. Please email eigen-core-team @ lists.tuxfamily.org if you need an account.
Bug 949 - Add static assertion for incompatible scalar types in decompositions
Summary: Add static assertion for incompatible scalar types in decompositions
Status: RESOLVED FIXED
Alias: None
Product: Eigen
Classification: Unclassified
Component: LU (show other bugs)
Version: 3.2
Hardware: All All
: High Wrong Result
Assignee: Nobody
URL:
Whiteboard:
Keywords:
: 750 (view as bug list)
Depends on: 750
Blocks: 3.3
  Show dependency treegraph
 
Reported: 2015-02-08 22:04 UTC by sebastien.varrette
Modified: 2015-03-13 22:34 UTC (History)
3 users (show)



Attachments
Demo (2.50 KB, text/x-csrc)
2015-02-08 22:04 UTC, sebastien.varrette
no flags Details
Static assertions on wrong scalar types (3.23 KB, patch)
2015-03-09 17:07 UTC, Gael Guennebaud
no flags Details | Diff
Static assertions on wrong scalar types (22.05 KB, patch)
2015-03-09 21:36 UTC, Gael Guennebaud
no flags Details | Diff

Description sebastien.varrette 2015-02-08 22:04:27 UTC
Created attachment 537 [details]
Demo

For a regular square matrix with n >= 5 rows, the determinant computation returns completely wrong results. I attach a sample code.

For instance for the following matrix: 

```
Eigen::Matrix<int,5,5> C0, C1, C2;
    C0 <<
        1, 2, 3, 4, 5,
        3, 0, 4, 5, 6,
        2, 1, 2, 3, 4,
        0, 0, 0, 6, 5,
        0, 0, 0, 5, 6;
    C1 <<
        0, 0,  3, 1, 1,
        3, 1,  1, 0, 1,
        1, 0,  2, 3, 3,
        3, 1,  2, 3, 3,
        0, 2,  1, 2, 1;
    C2 <<
	1, 1,  1,  1,  1,
	1, 2,  3,  4,  5,
	1, 3,  6, 10, 15,
	1, 4, 10, 20, 35,
	1, 5, 15, 35, 69;
```
Expected det(C0) = 99  (when C0.determinant() = -432
Expected det(C1) = -25 (when C1.determinant() = -54
Expected det(C2) = 0   (when C2.determinant() = 1296
Comment 1 Jitse Niesen 2015-02-09 08:13:03 UTC
Without looking at the code or doing any tests, I would say that the problem is probably that the scalar type is int. One needs division to do an LU decomposition and integer division is not a proper division. We should add a compile-time error to cover this case.
Comment 2 sebastien.varrette 2015-02-09 17:15:47 UTC
Ah ok, it definitively makes sense ;) 
For sure a raised error would help.
Comment 3 Gael Guennebaud 2015-03-09 17:07:57 UTC
Created attachment 555 [details]
Static assertions on wrong scalar types

This patch adds static assertions on the input scalar types of a dozen of dense solvers.
Comment 4 Christoph Hertzberg 2015-03-09 20:43:02 UTC
(In reply to Gael Guennebaud from comment #3)
> Created attachment 555 [details]

This appears to be part of the patch to Bug 973?
Comment 5 Gael Guennebaud 2015-03-09 21:36:06 UTC
Created attachment 556 [details]
Static assertions on wrong scalar types

oops, that one should be right.
Comment 6 Christoph Hertzberg 2015-03-13 21:05:50 UTC
*** Bug 750 has been marked as a duplicate of this bug. ***
Comment 7 Christoph Hertzberg 2015-03-13 21:11:03 UTC
(In reply to Gael Guennebaud from comment #5)
> Created attachment 556 [details]
> Static assertions on wrong scalar types

Patch looks good. Probably, we should also check in the default constructors to catch errors as early as possible.
Comment 8 Gael Guennebaud 2015-03-13 21:24:20 UTC
https://bitbucket.org/eigen/eigen/commits/678c42a8a42a/
Changeset:   678c42a8a42a
User:        ggael
Date:        2015-03-13 20:06:20+00:00
Summary:     Bug 949: add static assertion for incompatible scalar types in dense end-user decompositions.

Do we want to assert on helper decompositions too (Triadiagonal, Hessenberg, etc.) ??
Comment 9 Christoph Hertzberg 2015-03-13 22:06:26 UTC
I think we don't have to add it this to Tridiagonal, etc. as long as they are not supposed to be called from user code. It wouldn't hurt much either, I guess.

Shall we backport this to 3.2 (what the original report was for)?
Comment 10 Gael Guennebaud 2015-03-13 22:34:43 UTC
(In reply to Christoph Hertzberg from comment #9)
> Shall we backport this to 3.2 (what the original report was for)?

done:
https://bitbucket.org/eigen/eigen/commits/c24144a314e4/

For the other decompositions if anyone want to do it, go ahead.

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