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

remove 'clever' NA checks #790

Merged
merged 1 commit into from
Dec 20, 2017
Merged

remove 'clever' NA checks #790

merged 1 commit into from
Dec 20, 2017

Conversation

kevinushey
Copy link
Contributor

This PR removes the 'clever' NA checking code I implemented some time ago (way back in 2014). I recall benchmarking these as a bit faster than R's own NA checking routines, but I either I was mistaken or compilers have matured such that the performance difference no longer exists:

#include <Rcpp.h>
using namespace Rcpp;

// [[Rcpp::export]]
LogicalVector is_na(NumericVector x)
{
    std::size_t n = x.size();
    LogicalVector output = no_init(n);
    for (std::size_t i = 0; i < n; i++)
        output[i] = Rcpp::internal::Rcpp_IsNA(x[i]);
    return output;
}

/*** R
library(microbenchmark)

rnd <- rnorm(1E6)
rnd[sample(1:1E6, 1E5)] <- NA
identical(is.na(rnd), is_na(rnd))
microbenchmark::microbenchmark(
    R = is.na(rnd),
    Rcpp = is_na(rnd)
)
*/

I see, using the current Rcpp Rcpp_IsNA implementation,

> microbenchmark::microbenchmark(
+     R = is.na(rnd),
+     Rcpp = is_na(rnd)
+ )
Unit: microseconds
 expr     min      lq     mean   median       uq      max neval cld
    R 551.458 649.605 1397.701  997.121 1179.275 5433.238   100   a
 Rcpp 626.599 767.000 1412.142 1031.284 1219.745 5157.141   100   a

So let's just get rid of the old Rcpp code and delegate back to R's own NA checkers.

@eddelbuettel
Copy link
Member

Thumbs up for simpler is better.

I see the removal, but do we need a fill-in to delegate the old access points back to something else? Or do I need more coffee as I am missing something obvious?

@kevinushey
Copy link
Contributor Author

I don't think we need to fill in anything -- the old parts removed were implementation details living in an internal namespace; any client packages that were using those functions were doing so shouldn't have (and I don't think any packages were).

The APIs Rcpp_IsNA and Rcpp_IsNaN still exist; they just delegate directly to their R counterparts, so nothing has changed on that front.

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.

None yet

2 participants