This bugzilla service is closed. All entries have been migrated to https://gitlab.com/libeigen/eigen

Bug 1516

Summary: The module Eigen::matrix.diagonal(x) not giving right result after compilation when x>no_of_column
Product: Eigen Reporter: Gaurav <gaurav.gini34>
Component: Core - generalAssignee: Nobody <eigen.nobody>
Status: RESOLVED FIXED    
Severity: Internal Design CC: chtz, gael.guennebaud, gaurav.gini34, jacob.benoit.1, mayank007.win
Priority: Normal Keywords: JuniorJob, test-needed
Version: 3.3 (current stable)   
Hardware: All   
OS: All   
Whiteboard:
Bug Depends on:    
Bug Blocks: 558    
Attachments:
Description Flags
Added eigen_assert() to MatrixBase::diagonal(Index)
none
Added eigen_assert() to constructor of class Diagonal. none

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: https://bitbucket.org/eigen/eigen/pull-requests/373

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.,
https://bitbucket.org/eigen/eigen/src/306356e228ec/test/diagonal.cpp?at=default&fileviewer=file-view-default#diagonal.cpp-71

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.

default: https://bitbucket.org/eigen/eigen/commits/c6f36208dce2/
3.3:     https://bitbucket.org/eigen/eigen/commits/783d38b3c78c/

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 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/1516.