Re: [eigen] Index types change pushed

[Gael Guennebaud <gael.guennebaud@xxxxxxxxx>]

On Wed, Jun 2, 2010 at 7:48 PM, Benoit Jacob <jacob.benoit.1@xxxxxxxxx> wrote:

2010/6/2 Gael Guennebaud <gael.guennebaud@xxxxxxxxx>:
>
>
> On Wed, Jun 2, 2010 at 6:39 PM, Benoit Jacob <jacob.benoit.1@xxxxxxxxx>
> wrote:
>>
>> 2010/6/2 Gael Guennebaud <gael.guennebaud@xxxxxxxxx>:
>> >
>> > if you don't mind I would slightly change the implementation details to
>> > propagate the Index type trough the traits structure. Indeed, this is
>> > needed
>> > to add such a template paramter to the sparse matrix.
>>
>> I don't mind if you have to, but the ei_traits already propagates
>> StorageKind, from which you can get the Index type via ei_index, see
>> how it's done in e.g. DenseBase:
>>
>>    typedef typename ei_traits<Derived>::StorageKind StorageKind;
>>    typedef typename ei_index<StorageKind>::type Index;
>>
>> Do you think that this doesn't cover your needs in the Sparse module?
>>
>> Note that if you want to allow the index types to me more configurable
>> in the Sparse module, you can introduce more storage kinds. You could
>> even make them templated. Currently we have a single Sparse type for
>> StorageKind, but that could become Sparse<SomeIndexType> for example.
>> So I was considering the current system to be very flexible.
>
> this is a bit overkill and it actually does not work because the template
> parameter allowing to specialize the _expression_ implementations has to be a
> type not a templated struct...

I meant pass the type Sparse<short int>, not the template<typename
Index> Sparse. So this would work!

look at that example:

template<typename UnaryOp, typename MatrixType>
class CwiseUnaryOpImpl<UnaryOp,MatrixType,Sparse>
  : public SparseMatrixBase<CwiseUnaryOp<UnaryOp, MatrixType> >
{
...
};

we don't want to have to specialize CwiseUnaryOpImpl for each supported index types....

 

Also notice that the Index type has to be a property of the
StorageKind anyway. Indeed, remember that one of our initial
motivations for StorageKind was to eventually allow distributed
storage, and that would want 'long long' as indices.

I disagree, sparse matrix is a typical counter example where the index type plays a similar role than the scalar type.
 

That said, I understand that what you propose (having the Index type
propagated directly through expressions independently of the
StorageKind) can work too; I just think that the current design (of
letting the Index type be inferred from the StorageKind, and only
propagating the StorageKind) is a bit simpler.

Do as you think is right :)

already half done ;)

gael
 

Benoit

>
> Propagating the Index type as the scalar type sounds more natural to me.
>
> gael
>
>>
>> Benoit
>>
>> >
>> > gael
>> >
>> > On Sun, May 30, 2010 at 10:10 PM, Benoit Jacob
>> > <jacob.benoit.1@xxxxxxxxx>
>> > wrote:
>> >>
>> >> Hi list,
>> >>
>> >> The huge "index types" change is pushed to the development branch.
>> >>
>> >> *** What changed ***
>> >>
>> >> Before, we used type int for all indices. A recent huge thread on this
>> >> list showed that while the choice of a signed type was sensible, it
>> >> would be good to make it more adaptable. This is what this change
>> >> does:
>> >>  - dense matrices now default to std::ptrdiff_t, which means that
>> >> you're sure to be able to address half the size of your address space
>> >> (half, because it's signed).
>> >>  - sparse matrices are still using int, because the sizeof() matters a
>> >> lot there (since we are story arrays of indices).
>> >>
>> >> You can change that at will by defining EIGEN_DEFAULT_DENSE_INDEX_TYPE
>> >> and EIGEN_DEFAULT_SPARSE_INDEX_TYPE. Of course, this breaks ABI
>> >> compatibility.
>> >>
>> >> In the future we'll probably add an option to Sparse matrix types to
>> >> specify different index types. On the other hand, for Dense matrices,
>> >> I don't think there can ever be a valid reason to use something else
>> >> than ptrdiff_t. Notice that for fixed-size matrices, this doesn't have
>> >> any cost as the sizes aren't stored as runtime variables.
>> >>
>> >> *** How this works ***
>> >>
>> >> This works per-StorageKind. So if in the future we add a new
>> >> StorageKind for "Dense matrix stored over distributed storage", we're
>> >> free to specify e.g. long long int for them, without affecting the
>> >> rest.
>> >>
>> >> See in XprHelper.h, the new struct ei_index, see this code:
>> >>
>> >>
>> >> template<typename StorageKind> struct ei_index {};
>> >>
>> >> template<>
>> >> struct ei_index<Dense>
>> >> { typedef EIGEN_DEFAULT_DENSE_INDEX_TYPE type; };
>> >>
>> >> typedef typename ei_index<Dense>::type DenseIndex;
>> >>
>> >>
>> >> Then in the macro EIGEN_DENSE_PUBLIC_INTERFACE, we now have:
>> >>
>> >>  typedef typename Eigen::ei_traits<Derived>::StorageKind StorageKind; \
>> >>  typedef typename Eigen::ei_index<StorageKind>::type Index; \
>> >>
>> >> this means that all expressions have a Index typedef that means the
>> >> right thing. Normally, we propagate this recursively as appropriate.
>> >> However there are some contexts where we know we are only dealing with
>> >> Dense expressions, and don't have a MatrixType to work with (e.g. in
>> >> some public functions). There, it's OK to specify explicitly
>> >> DenseIndex.
>> >>
>> >> Then, we have the same thing in the Sparse module, in SparseUtil.h:
>> >>
>> >> template<>
>> >> struct ei_index<Sparse>
>> >> { typedef EIGEN_DEFAULT_SPARSE_INDEX_TYPE type; };
>> >>
>> >> typedef typename ei_index<Sparse>::type SparseIndex;
>> >>
>> >>
>> >> etc, etc.
>> >>
>> >> Benoit
>> >>
>> >>
>> >
>> >
>>
>>
>
>