Bugzilla – Bug 579
Missing size parameters in fixed-size block method
Last modified: 2013-12-21 21:51:56 UTC
Created attachment 325 [details]
Patch to solve the issue
I need to make blocks of partly dynamic matrices, something along the line of the following:
template <int Rows>
void f(int rows)
Array<float, Rows, Dynamic> a(rows, 1234);
Array<float, Rows, 4> b;
a.block(0, 567, rows, 4) += b;
a.block<Rows, 4>(0, 567, rows, 4) += b; // *** BREAKS!
The second line doesn't compile, since the method block<R,C>(i,j) takes only two arguments, probably due to oversight. Fixing it is trivial, I have attached a patch.
(In reply to comment #0)
> The second line doesn't compile, since the method block<R,C>(i,j) takes only
> two arguments, probably due to oversight. Fixing it is trivial, I have attached
> a patch.
I agree this needs to be fixed, however I think it should be allowed only to pass either both sizes or none. Otherwise the following might lead to confusing run-time errors:
a.block<4, Eigen::Dynamic>(0,0, 4);
i.e. if a user assumes only the dynamic dimension needs to be specified.
Furthermore, it should be possible to make compile-time checks that dimensions are required if one of the dimensions is dynamic.
And another thing: Basically the same needs to be implemented for all
top/bottom##Left/Right##Corner() methods. Maybe the whole thing can be abbreviated with some preprocessor-voodoo. (Especially, since furthermore all is required as const and non-const)
Created attachment 349 [details]
implementation example of partial fixed-size blocks
Here is an implementation for block(). I don't think we should enforce that one of the template argument is Dynamic. Might be useful in generic code.
This patch should be extended to other similar methods and the block.cpp unit test have to be extended respectively. Code snippets as for the others would be nice too. Any volunteer?
Done. I decided against preprocessor voodoo as I'm afraid of it backfiring.
date: Tue Jun 18 14:29:15 2013 +0100
summary: Implement mixed static/dynamic-size .block() (bug 579)
thanks a lot for handling this quite repetitive task.
Created attachment 405 [details]
Add run-time params for more compile-size methods
As detailed in the mailing list , some fixed-size methods were overseen when fixing this bug. The attached patch closes this gap.
(In reply to comment #5)
> Created attachment 405 [details] [review]
> Add run-time params for more compile-size methods
The patch is ok for the additional methods, however you should not remove the variants where two parameters are templated. The reason is that expressions like the following should be caught at compile-time:
A.topLeftCorner<2, Dynamic>(3); // wrong --> runtime error
A.topLeftCorner<2, Dynamic>(2, 3); // probably this was intended
I.e. a programmer might think that only the dynamic sizes must be specified.
Created attachment 407 [details]
Add run-time params for more compile-size methods, v2
Agreed. I thought this was a non-issue, and it is at least for methods with only one template parameter, since e.g. head<Dynamic>(n) will probably only be used in generic code. You are right however about the case with two parameters. Fixed.
Imported to 3.2 and grafted to default (that required some manual resolving, due to the EIGEN_DEVICE_FUNC)