Fix gprod for integer64

[ben-schwen]

Closes #5225.

[codecov]

Codecov Report

Merging #5231 (b4f8348) into master (e88826e) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #5231   +/-   ##
=======================================
  Coverage   99.50%   99.50%           
=======================================
  Files          77       77           
  Lines       14590    14603   +13     
=======================================
+ Hits        14518    14531   +13     
  Misses         72       72           

Impacted Files	Coverage Δ
src/gsumm.c	100.00% <100.00%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e88826e...b4f8348. Read the comment docs.

[mattdowle]

Great spot and fix. It's certainly debatable, but keeping the spirit of base::prod always returning double is how gforce prod is done for regular integer and I'm thinking that makes sense for integer64 too. Despite integer64 having its own prod method. That could change in bit64 in future possibly. Can't really explain it very well, but what I've changed it to feels right to me. It's testing against fixed y values that are a bit more independent of bit64, and there's a reason to do this in that we like and agree with (in this particular case) base::prod always returning double.
Ok with you?

[MichaelChirico]

not advocating one way or the other, but just noting that one difference of int vs int64 is that there are values of int64 that can't be represented exactly in double right? IIRC somewhere like 2^53 where doubles no longer have integer-level precision. whereas that's not true for int -- all valid int can be represented as double.

so it seems like returning int64 could me more correct in some cases, IIUC

[ben-schwen]

Perfectly fine for me. The only problem I see arising then is that different optimization levels will produce different outputs, see also my comment here.

library(bit64)
DT = data.table(x=c(lim.integer64(), 1, 1), g=1:2)
options(datatable.optimize=0L)
DT[, prod(x), g]
#>    g                   V1
#> 1: 1 -9223372036854775807
#> 2: 2  9223372036854775807
options(datatable.optimize=2L)
DT[, prod(x), g]
#>    g            V1
#> 1: 1 -9.223372e+18
#> 2: 2  9.223372e+18

My worries are not about losing precision but about the different types of the different optimization levels. This can become an even bigger problem for the user when certain functions turn off gforce optimization as e.g. DT[, c(prod(x)), g]

[mattdowle]

Great points. Ok returning integer64 it is then.