AndriSignorell / DescTools

Tools for Descriptive Statistics and Exploratory Data Analysis
http://andrisignorell.github.io/DescTools/
82 stars 18 forks source link

LCM integer overflow #128

Closed NicChr closed 7 months ago

NicChr commented 8 months ago

Hello, I noticed that LCM() is silently producing integer overflows. Please see the below reprex.

library(DescTools)

packageVersion("DescTools")
#> [1] '0.99.50.3'

lcm_1_22 <- DescTools::LCM(1:22)
lcm_1_22
#> [1] 232792560

cat("Result:", DescTools::LCM(lcm_1_22, 23L))
#> Result: 1059261584
cat("Expected:", (lcm_1_22 / DescTools::GCD(lcm_1_22, 23)) * 23)
#> Expected: 5354228880

Created on 2023-11-18 with reprex v2.0.2

AndriSignorell commented 8 months ago

@NicChr: Thanx for this hint. You are exceeding the integer value range of 2^31. With this in mind, I looked for and found more modern alternatives. C++ version 17 allows calculation with long integers. This only postpones the problem, but for our generation it should be enough ... ;-) Please check and close the task, if ok.

jtlevine commented 7 months ago

@AndriSignorell I think this change may have broken the most recent release of this package for R versions >=4.0.0 and <4.3.0. Like you said the gcd and lcm functions were introduced in C++17, however R versions prior to 4.3.0 still default to C++14

AndriSignorell commented 7 months ago

Thanks for this hint @jtlevine. I wasn't aware of this, so I will upgrade the requirements to R >= 4.3.0 and resubmit. Time to move on anyway.... ;-) The new version is by the way also 20% faster than the boost implementation.

AndriSignorell commented 7 months ago

Fixed:

packageVersion("DescTools")
#> [1] '0.99.52.1'

lcm_1_22 <- DescTools::LCM(1:22)
lcm_1_22
[1] 232792560
#> [1] 232792560

cat("Result:", DescTools::LCM(lcm_1_22, 23L))
#> Result: 5354228880> 
cat("Expected:", (lcm_1_22 / DescTools::GCD(lcm_1_22, 23)) * 23)
#> Expected: 5354228880
AndriSignorell commented 7 months ago

@NicChr: would you mind checking this and close the issue, if ok?

NicChr commented 7 months ago

Thanks @AndriSignorell, looks good now!

Zhenglei-BCS commented 1 month ago

Sorry to re-open the issue, for people still using R version smaller than 4.3.0, is there a general instruction on how to install the older compatible version of DescTools?