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.
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() ??
sure, i am new to open source and was looking for bugs to fix. i will start off with this.
Is the Bug still open ?
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.
Ok. So I should go with static_assert or eigen_assert ??
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.
Created attachment 833 [details] Added eigen_assert() to MatrixBase::diagonal(Index)
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.
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`
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.
Created attachment 837 [details] Added eigen_assert() to constructor of class Diagonal.
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 ?
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.
-- 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.