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 <[email protected]> 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 > > > > > > > > > > > > > > > > > > > > > >
