New user self-registration is disabled due to spam. Please email eigen-core-team @ lists.tuxfamily.org if you need an account.
Before reporting a bug, please make sure that your Eigen version is up-to-date!
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...
Status: RESOLVED FIXED
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
URL:
Whiteboard:
Keywords: JuniorJob, test-needed
Depends on:
Blocks: 3.3
  Show dependency treegraph
 
Reported: 2018-02-08 19:40 UTC by Gaurav
Modified: 2018-04-03 14:18 UTC (History)
5 users (show)



Attachments
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: 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.

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