Re: [eigen] Re: Today's joke

[Benoit Jacob <jacob.benoit.1@xxxxxxxxx>]

2010/6/11 Gael Guennebaud <gael.guennebaud@xxxxxxxxx>:
>
>
> 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.

Ah yes, at some point I thought about that use case, but forgot.

What we can do is adopt explicit names, for what these macros are
doing. One is preferring Dynamic, the other is preferring fixed
values. Based on that, a good English speaker might be able to come up
with good names :)

Naive attempt:
  EIGEN_SIZE_MIN ---> EIGEN_MIN_PREFER_DYNAMIC
  EIGEN_SIZE_MIN ---> EIGEN_MIN_PREFER_FIXED

Benoit

>
> 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
>> >
>>
>>
>
>