Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make tweaks for CRAN #132

Merged
merged 3 commits into from
Feb 14, 2018
Merged

Make tweaks for CRAN #132

merged 3 commits into from
Feb 14, 2018

Conversation

lukesonnet
Copy link
Contributor

@lukesonnet lukesonnet commented Feb 14, 2018

There were some "deep" CRAN errors we needed to resolve that you can see here: https://cran.r-project.org/web/checks/check_results_estimatr.html

  1. Tests failed without Long Doubles (https://www.stats.ox.ac.uk/pub/bdr/noLD/estimatr.out). They were not identical but rather equal, so I just changed the test.
  2. Uninitialized values (the degrees of freedom, residuals, and residual variance in lm_robust_helper.cpp were being used and returned to the R wrapper code and were causing errors in Valgrind. I now initialize these values and tested all of the test files one by one using valgrind and there are no more errors.
  3. Solaris errored on a test that expect_equal the max diff of two margins was less than < 2e05. I changed the test to expect_equals with a tolerance of 0.01. I have no way to test this since rhub's Solaris platform apparently cannot compile RcppEigen files (and I'm not sure if I can fix this). See rhub Solaris fails to compile RcppEigen package r-hub/rhub#109.

I think if this passes, we might as well try to resubmit and see what they say?

Oh, I also got rid of my silly way to handle if the cholesky decomposition failed that relied on catching my own error. Logic should be much cleaner now.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.0002%) to 99.633% when pulling a7c10c8 on lukesonnet/cran-iss into ad8f606 on master.

@lukesonnet
Copy link
Contributor Author

Sorry @nfultz , I'm not sure where a lot of the diffs in the cpp came from, most are useless.

Copy link
Contributor

@nfultz nfultz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you adding do_qr - flow control via exceptions is unfun.

@nfultz
Copy link
Contributor

nfultz commented Feb 14, 2018

LGTM, feel free to merge

@lukesonnet lukesonnet merged commit f1e0deb into master Feb 14, 2018
@lukesonnet lukesonnet deleted the lukesonnet/cran-iss branch February 19, 2018 18:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants