Re: [eigen] Re: Today's joke

On Fri, Jun 11, 2010 at 2:37 PM, Benoit Jacob <jacob.benoit.1@xxxxxxxxx> wrote:

This is now checked in: the value of Dynamic has been changed to -1.

A couple of practical consequences:
- be careful that expressions like RowsAtCompileTime<=4 are now true
in the Dynamic case, so make sure to check explicitly for Dynamic.
- when writing loop meta-unrollers, use 0, not -1, as the stop case.

This was an occasion to sanitize some code: the macros
EIGEN_ENUM_MIN/MAX were abused, since they never were fit to check
compile-time sizes where the special values Dynamic, 0, and 1 need to
be special-cased. So they have been renamed to
EIGEN_PLAIN_ENUM_MIN/MAX, the EIGEN_SIZE_MIN macro has been polished,
and a new macro has been introduced: EIGEN_MAXSIZE_MIN. Explanation:
when you try to get the min between some fixed value, say, 4, and
Dynamic, do you expect the min to be 4 or Dynamic ? If you are
checking something like RowsAtCompileTime, then it should be Dynamic,
because a dynamic block in a 4x4 matrix has a Dynamic number of rows.
But if you are checking something like MaxRowsAtCompileTime, then you
want the min to be 4. Because a dynamic block in a 4x4 matrix has at
most 4 rows. In conclusion:
- use EIGEN_SIZE_MIN when comparing sizes-at-compile-time. It treats
Dynamic as a small value. Only 0 and 1 are treated as even smaller
than Dynamic, as special cases.
- use EIGEN_MAXSIZE_MIN when comparing max-sizes-at-compile-time. It
treats Dynamic as a large value.

I'm not sure about these rules. Actually it seems to me that such a EIGEN_SIZE_MIN is meant for very specific cases. For instance there are many lines like:

InnerSize = EIGEN_SIZE_MIN(_LhsNested::ColsAtCompileTime, _RhsNested::RowsAtCompileTime)

which now are wrong, because here since the sizes are compatible the min of Dynamic and 4 has to be 4. So Perhaps, EIGEN_SIZE_MIN should be renamed to a more explicit one, but I have no good ideas.

gael

Benoit

2010/5/30 Benoit Jacob <jacob.benoit.1@xxxxxxxxx>:

> Hi again,
>
> so, while doing the index types change, I realized something that's
> going to make me take a more humble approach in my rationalizations in
> the future :)
>
> Many people asked why the strange value 33331 for the constant
> Dynamic. An explanation is given in the code (see Constants.h), but
> that basically has a lot to do with the assumption that indices are at
> least 32bit wide. Of course, since today, that is no longer always
> true. For now the only valid use case for changing that is in the
> Sparse module, where we don't use that Dynamic value much, but still,
> it's unsafe. And since it's a usual template parameter value for
> Matrix, it impacts a lot our ABI. In short, once Eigen 3.0 is
> released, we can't tweak that value anymore. So having it rely on
> technical details is not comfortable.
>
> So i think after all I'll follow people's sound advice and change it to -1 !!!!
>
> My big argument against that is that e.g. (RowsAtCompileTime <= 4)
> will no longer mean what it seems to (it will be true in the Dynamic
> case !!!) but that is fortunately not a kind condition that we use too
> frequently in the code. Should be a reasonably small change.
>
> Benoit
>