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.,
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.
I allowed empty diagonals, otherwise we might break existing valid code.