radfordneal / pqR

pqR - a "pretty quick" version of R
GNU General Public License v2.0
238 stars 16 forks source link

Update formula does not work as expected #40

Open mardukbp opened 5 years ago

mardukbp commented 5 years ago

In Gnu-R 2.15 and 3.6 update(~1, ~. - y) produces ~1. pqR produces ~1 - 1

radfordneal commented 5 years ago

Thanks for the report.

I've investigated, and found that the bug is also present in R-2.15.1. It was fixed in R-3.1.1, which reported it as affecting formulas with exactly 32 variables (PR 15735). I've incorporated the R-3.1.1 fix in the development version of pqR. I may put out a minor update with this and other fixes, or it may wait until the new version with automatic differentiation is ready.

mardukbp commented 5 years ago

Thank you Radford for investigating and fixing this bug. I would say it is better to publish a minor update, which includes this and other bug fixes. That is the purpose of minor updates. And it would allow more people to use pqR as a drop-in replacement for GNU-R.

I came across this project while trying out Oracle's FastR (a JVM-based R implementation) to speed up the performance of the oSCR package. In my only benchmark pqR turned out to be twice as fast as GNU-R and 1.5 times as fast as FastR. Now a friend of mine is using pqR for his research.

I read in the Issues that your enhancements to R are unlikely to be accepted by the R Core Team, which is a pity (to say the least). I suggested to the main FastR developer to incorporate some of your optimizations. It would be great for your hard work to get more recognition and make a wider impact now that R is so fashionable for Big Data and ML. I would also like to note that FastR can be coupled with GraalVM's visual profiler, which makes it pretty easy to detect hot spots in an R package.

nuest commented 5 years ago

@mardukbp If you still wanna try out this fix in a Dockerfile: Clone https://github.com/nuest/pqr-docker an then run

$ REF=cbc944dabe0818875f99afe298f09430ecb450a2; docker build --tag pqr:$(echo $REF) --build-arg REF=$REF --file dev/Dockerfile .
$ docker run --rm -it pqr:cbc944dabe0818875f99afe298f09430ecb450a2 

pqR version 2.15.1 (2019-00-00), based on R 2.15.0 (2012-03-30)

R 2.15.0 is Copyright (C) 2012 The R Foundation for Statistical Computing
ISBN 3-900051-07-0

Modifications to R in pqR are Copyright (C) 2013-2019 Radford M. Neal

Some modules are from R-2.15.1 or later versions distributed by the R Core Team

Platform: x86_64-unknown-linux-gnu (64-bit)

R is free software and comes with ABSOLUTELY NO WARRANTY.
You are welcome to redistribute it under certain conditions.
Type 'license()' or 'licence()' for distribution details.

R is a collaborative project with many contributors.
Type 'contributors()' for more information and
'citation()' on how to cite R or R packages in publications.

Type 'demo()' for some demos, 'help()' for on-line help, or
'help.start()' for an HTML browser interface to help.
Type 'q()' to quit R.

Used CRAN mirror:  ftp://price.utstat.utoronto.ca/2019-02-19 

No helper threads, task merging enabled, uncompressed pointers.

> sessionInfo()
R version 2.15.1 (2019-00-00)
Platform: x86_64-unknown-linux-gnu (64-bit)

locale:
[1] C

attached base packages:
[1] stats     graphics  grDevices utils     datasets  methods   base     
> update(~1, ~. - y)
~1
nuest commented 5 years ago

Comparison to the commit before the fix:

$ REF=21b7226f26524933ff88ea6335bf807e9c768630; docker build --tag pqr:$(echo $REF) --build-arg REF=$REF --file dev/Dockerfile .
$ docker run --rm -it pqr:21b7226f26524933ff88ea6335bf807e9c768630 Rscript -e 'update(~1, ~. - y)'

Used CRAN mirror:  ftp://price.utstat.utoronto.ca/2019-02-19 
~1 - 1
mardukbp commented 5 years ago

Thank you Daniel! I already tried the fix on my laptop and on an AWS EC2 instance. Your Docker file would make it easy for people to deploy pqR on AWS. I think this use case should be advertised on the project's website.

nuest commented 5 years ago

I have no experience with AWS, so if you have a small code chunk or example configuration, I'd be very happy about a PR to the README :-).