This bugzilla service is closed. All entries have been migrated to
Bug 1516 - The module Eigen::matrix.diagonal(x) not giving right result after compilation when x>no_of_column
Summary: The module Eigen::matrix.diagonal(x) not giving right result after compilatio...
Alias: None
Product: Eigen
Classification: Unclassified
Component: Core - general (show other bugs)
Version: 3.3 (current stable)
Hardware: All All
: Normal Internal Design
Assignee: Nobody
Keywords: JuniorJob, test-needed
Depends on:
Blocks: 3.3
  Show dependency treegraph
Reported: 2018-02-08 19:40 UTC by Gaurav
Modified: 2019-12-04 17:27 UTC (History)
5 users (show)

Added eigen_assert() to MatrixBase::diagonal(Index) (1.15 KB, patch)
2018-03-12 20:27 UTC, Mayank Agarwal
no flags Details | Diff
Added eigen_assert() to constructor of class Diagonal. (1.28 KB, patch)
2018-03-20 08:41 UTC, Mayank Agarwal
no flags Details | Diff

Description Gaurav 2018-02-08 19:40:52 UTC
There should be a compilation error if the module is not supposed to work for x>no_of_column instead it gives a result window with all the information.
Comment 1 Gael Guennebaud 2018-02-09 10:24:42 UTC
Right, it seems that assertions are missing. Are you willing to propose a patch adding the appropriate assertions (using the eigen_assertion(...) macro) in MatrixBase::diagonal(Index) the and respective unit tests in test/diagonal.cpp/diagonal_assert() ??
Comment 2 Gaurav 2018-02-09 20:31:10 UTC
sure, i am new to open source and was looking for bugs to fix. i will start off with this.
Comment 3 Mayank Agarwal 2018-03-06 17:18:12 UTC
Is the Bug still open ?
Comment 4 Christoph Hertzberg 2018-03-07 10:20:09 UTC
I don't see that any fix was provided yet. If Gaurav is not responding that he is still working on it, feel free to provide a patch/pull request.
Comment 5 Mayank Agarwal 2018-03-12 15:20:30 UTC
Ok. So I should go with static_assert or eigen_assert ??
Comment 6 Christoph Hertzberg 2018-03-12 16:13:40 UTC
I'd suggest just making a simple `eigen_assert` for now.
We could make additional static asserts, if it is known at compile-time that the size can't fit (this would actually also be true for all `.block<...>` methods.
Comment 7 Mayank Agarwal 2018-03-12 20:27:08 UTC
Created attachment 833 [details]
Added eigen_assert() to MatrixBase::diagonal(Index)
Comment 8 Mayank Agarwal 2018-03-12 20:33:28 UTC
I have added the patch here and also made a pull request:

Have just started with Eigen, let me know if I did it the wrong way or if
there's something else to do.
Also I missed the unit tests, I was going through it now, but the documentation on writing unit tests, says to have a look at the other unit test files in eigen/test to fill the test_mytest() function. I am not getting on how I should write the unit test.
Comment 9 Christoph Hertzberg 2018-03-13 07:45:01 UTC
For the unit tests, check e.g.,

Regarding your patch: You should check against .rows() and .cols() -- MaxColsAtCompileTime could be -1 or it could be larger than the actual matrix.
Also, you don't check every variant of .diagonal() yet. Maybe put the assert into the constructor of `class Diagonal`
Comment 10 Christoph Hertzberg 2018-03-13 14:24:22 UTC
One decision which has to be made:
Should Matrix3d().diagonal(3) assert or should it result in an empty matrix/vector?

I can imagine there are cases where the latter behavior could avoid implementing special cases.
Comment 11 Mayank Agarwal 2018-03-20 08:41:41 UTC
Created attachment 837 [details]
Added eigen_assert() to constructor of class Diagonal.
Comment 12 Mayank Agarwal 2018-03-20 08:46:55 UTC
Have added a new attachment, please review.
Also Matrix3d().diagonal(3) will return a empty matrix/vector as of now, since it could avoid implementing special cases as you said. May be some discussion is required on it, So for now it will return empty matrix/vector.

Also I am going through the test cases on how to implement them. I should make a new file or should add test cases to test/diagonal.cpp ?
Comment 13 Gael Guennebaud 2018-04-03 14:18:25 UTC
I ended up doing it myself. The test in the patch was wrong too.


I allowed empty diagonals, otherwise we might break existing valid code.
Comment 14 Nobody 2019-12-04 17:27:51 UTC
-- GitLab Migration Automatic Message --

This bug has been migrated to'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:

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