Bug 579 - Missing size parameters in fixed-size block method
Missing size parameters in fixed-size block method
Status: RESOLVED FIXED
Product: Eigen
Classification: Unclassified
Component: Core - general
unspecified
All All
: Normal Unknown
Assigned To: Nobody
:
Depends on:
Blocks: 3.2
  Show dependency treegraph
 
Reported: 2013-04-04 16:00 UTC by Marton Danoczy
Modified: 2013-12-21 21:51 UTC (History)
5 users (show)



Attachments
Patch to solve the issue (1.96 KB, application/octet-stream)
2013-04-04 16:00 UTC, Marton Danoczy
no flags Details
implementation example of partial fixed-size blocks (2.06 KB, patch)
2013-06-17 21:44 UTC, Gael Guennebaud
no flags Details | Diff
Add run-time params for more compile-size methods (29.55 KB, patch)
2013-12-16 19:27 UTC, Marton Danoczy
no flags Details | Diff
Add run-time params for more compile-size methods, v2 (23.12 KB, patch)
2013-12-17 01:07 UTC, Marton Danoczy
no flags Details | Diff

Description Marton Danoczy 2013-04-04 16:00:52 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.
Comment 1 Christoph Hertzberg 2013-04-08 13:15:14 UTC
(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)
Comment 2 Gael Guennebaud 2013-06-17 21:44:59 UTC
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?
Comment 3 Jitse Niesen 2013-06-18 15:37:52 UTC
Done. I decided against preprocessor voodoo as I'm afraid of it backfiring.

changeset:   5356:e1dfda41aec9
date:        Tue Jun 18 14:29:15 2013 +0100
summary:     Implement mixed static/dynamic-size .block() (bug 579)
Comment 4 Gael Guennebaud 2013-06-18 16:39:17 UTC
thanks a lot for handling this quite repetitive task.
Comment 5 Marton Danoczy 2013-12-16 19:27:35 UTC
Created attachment 405 [details]
Add run-time params for more compile-size methods

As detailed in the mailing list [1], some fixed-size methods were overseen when fixing this bug. The attached patch closes this gap.

[1]: http://listengine.tuxfamily.org/lists.tuxfamily.org/eigen/2013/12/msg00001.html
Comment 6 Christoph Hertzberg 2013-12-17 00:49:31 UTC
(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:

  Matrix<...> A;
  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.
Comment 7 Marton Danoczy 2013-12-17 01:07:25 UTC
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.
Comment 8 Christoph Hertzberg 2013-12-21 21:51:56 UTC
Imported to 3.2 and grafted to default (that required some manual resolving, due to the EIGEN_DEVICE_FUNC)

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