Re: [eigen] Matrix2i mean

[Gael Guennebaud <gael.guennebaud@xxxxxxxxx>]

Regarding the documentation, since mostly all operators/methods could easily overflow with integer types, I would suggest to rather document this general issue at a higher level. For instance, we could have a dedicated page explaining the differences between what happens when dealing with scalar types (e.g., int16+int16->int32) versus Eigen's matrices, and how to workaround this issue using .cast<>(). And then we could link to this page from Matrix/Array docs as well as from the first section of http://eigen.tuxfamily.org/dox/group__TutorialMatrixClass.html .

gael

On Wed, Nov 6, 2019 at 7:37 PM Petr Kubánek <pkubanek@xxxxxxxxx> wrote:

Patch attached. Hopefully clarifies the documentation. I will see if I
will find time to work on better solution.

Petr

On Tue, 2019-11-05 at 17:11 +0100, Christoph Hertzberg wrote:
> Adding an additional template method would not be a problem (minimal
> example: https://godbolt.org/z/ZB9WDI), though I don't see any real
> advantage vs writing
>
>      m.cast<int>().sum();
>
> Sure, `m.sum<int>();` would be shorter, but also more confusing.
>
> Your "another template parameter for the matrices" formulation
> sounded
> to me like you wanted to add another template parameter to
> `Eigen::Matrix` (which is not an option).
>
> Any patches clarifying the documentation are welcome!
>
> Christoph
>
>
> On 05/11/2019 16.44, Petr Kubánek wrote:
> > Hi,
> >
> > I know what's the problem. I am just looking either to document it
> > or
> > for a solution.
> >
> > Why would:
> >
> > template <typename dt> dt sum()
> >
> > break API/ABI? You will be able to call either sum() or
> > sum<int32_t>(),
> > shouldn't you? I will try that and submit a patch.
> >
> > Having sum<dt>() working, one can hopefully create mean<dt_sum,
> > dt_mean>(), so one can code:
> >
> > mean<int32_t,double>()
> >
> > Petr
> >
> > On Tue, 2019-11-05 at 16:36 +0100, Christoph Hertzberg wrote:
> > > On 05/11/2019 16.11, Peter wrote:
> > > > [...]
> > > >
> > > > actually, this would also be interesting for the scalar
> > > > products
> > > > in
> > > > general, namely a different
> > > > type for accumulating the sums within a scalar product, e.g. as
> > > > yet
> > > > another template parameter for the matrices.
> > >
> > > Adding another template parameter to basic types is not an
> > > option.
> > > This
> > > would break ABI and API compatibility (even if the parameter has
> > > a
> > > default).
> > >
> > > You could create your own custom type `my_int16` for which
> > > `my_int16
> > > *
> > > my_int16` results in a `my_int32` (this needs to be told to
> > > Eigen,
> > > similar to how real*complex products are handled).
> > >
> > > > > [...]
> > > >
> > > > I think it's more subtle than that.
> > > > Even
> > > >
> > > >     int16_t  Two =  2;
> > > >     int16_t  Max =  INT16_MAX;
> > > >     int16_t mean = ( Max/2 + Max + Two + Two ) / int16_t(4);
> > > >
> > > > doesn't produce an overflow.
> > >
> > > Yes, because `Max/2` gets implicitly converted to `int`.
> > > Actually,
> > > even
> > > adding two `int16_t` get implicitly converted to `int` (search
> > > for
> > > integer promotion rules -- I don't know them entirely either).
> > > And
> > > dividing an `int` by an `int16_t` results in an `int` which is
> > > only
> > > afterwards converted to `int16_t`.
> > >
> > > In contrast, Eigen::DenseBase::sum() does something (more or
> > > less)
> > > equivalent to:
> > >
> > >       T sum = T(0);
> > >       for(Index i=0; i< size(); ++i)
> > >           sum+=coeff(i);
> > >       return sum;
> > >
> > > i.e., after each addition the result gets reduced to the scalar
> > > type
> > > of
> > > the matrix, thus it will immediately overflow.
> > >
> > > And mean() essentially just takes the return value of `sum()` and
> > > divides it by the size.
> > >
> > >
> > > Christoph
> > >
> > >
> >
> >
> >
>
>