one less TODO item

Author: romainfrancois

ChangeLog

@@ -9,7 +9,12 @@
         * R/Rcpp.package.skeleton.R: Setting attributes to TRUE by default. This is
         what we should encourage people to use. 
         * include/Rcpp/as.h: add as<char> specialization
-	
+        * include/Rcpp/sugar/functions/diff.h : rework the implementation of diff
+        so that it works even when we don't know the previous value
+        * unitTests/runit.sugar.R : 
+        * unitTests/cpp/sugar.cpp :
+        * TODO : 2 less items
+        
 2013-09-17  JJ Allaire  <jj@rstudio.org>
 
 	* R/Attributes.R: Call inlineCxxPlugin and Rcpp.plugin.maker without

TODO

@@ -42,9 +42,6 @@ Syntactic sugar
 	maybe we should use LazyVector like in outer to somehow cache the
 	result when it is judged expensive to calculate
 
-    o   The current impl of "diff" might cause problems (e.g. with
-        ifelse) due to laziness, it is probably best to not make it lazy
-
     o 	crossprod
 
     o	Vector * Matrix, Matrix * Matrix

inst/include/Rcpp/sugar/functions/diff.h

@@ -1,8 +1,8 @@
-// -*- mode: C++; c-indent-level: 4; c-basic-offset: 4; tab-width: 8 -*-
+// -*- mode: C++; c-indent-level: 4; c-basic-offset: 4; tab-width: 4 -*-
 //
 // diff.h: Rcpp R/C++ interface class library -- diff
 //
-// Copyright (C) 2010 - 2012 Dirk Eddelbuettel and Romain Francois
+// Copyright (C) 2010 - 2013 Dirk Eddelbuettel and Romain Francois
 //
 // This file is part of Rcpp.
 //
@@ -24,42 +24,50 @@
 
 namespace Rcpp{
 namespace sugar{
-	
-	// NOTE: caching the previous value so that we only have to fetch the 
-	//       value once only works because we process the object from left to 
-	//       right
+
+// NOTE: caching the previous value so that we only have to fetch the 
+//       value once only works because we process the object from left to 
+//       right
 template <int RTYPE, bool LHS_NA, typename LHS_T>
 class Diff : public Rcpp::VectorBase< RTYPE, LHS_NA , Diff<RTYPE,LHS_NA,LHS_T> > {
 public:
 	typedef typename Rcpp::VectorBase<RTYPE,LHS_NA,LHS_T> LHS_TYPE ;
 	typedef typename Rcpp::traits::storage_type<RTYPE>::type STORAGE ;
 
-	Diff( const LHS_TYPE& lhs_ ) : lhs(lhs_), previous(lhs_[0]), was_na(false) {
-		was_na = traits::is_na<RTYPE>(previous) ;
-	}
+	Diff( const LHS_TYPE& lhs_ ) : 
+	    lhs(lhs_), 
+	    previous(lhs_[0]),
+	    previous_index(0),
+	    was_na(traits::is_na<RTYPE>(previous)) 
+	{}
 
 	inline STORAGE operator[]( int i ) const {
-		STORAGE y = lhs[i+1] ;
-		if( was_na ){
-			previous = y ;
-			was_na = traits::is_na<RTYPE>(y) ;
-			return previous ; // NA
-		}
-		if( traits::is_na<RTYPE>(y) ) {
-			was_na = true ;
-			previous = y ;
-			return previous ; // NA
-		}
-		STORAGE res = y - previous ;
-		previous = y ;
-		was_na = false ;
-		return res ;
+        STORAGE y = lhs[i+1] ;
+        if( previous_index != i ){
+            // we don't know the previous value, we need to get it. 
+            set_previous(i, lhs[i] ) ; // record the current value
+        }
+        if( was_na || traits::is_na<RTYPE>(y) ) {
+            set_previous(i+1, y ) ;
+            return traits::get_na<RTYPE>() ; // NA
+        }
+        STORAGE res = y - previous ;
+        set_previous( i+1, y) ;
+        return res ;
+	}     
+	
+	inline void set_previous(int i, STORAGE value){
+	    previous = value ;
+	    was_na = is_na<RTYPE>(previous) ;
+	    previous_index = i ;
 	}
+	
 	inline int size() const { return lhs.size() - 1 ; }
 
 private:
 	const LHS_TYPE& lhs ;
 	mutable STORAGE previous ;
+	mutable int previous_index ;
 	mutable bool was_na ;
 } ;
 
@@ -68,19 +76,22 @@ class Diff<REALSXP, LHS_NA, LHS_T> : public Rcpp::VectorBase< REALSXP, LHS_NA, D
 public:
 	typedef typename Rcpp::VectorBase<REALSXP,LHS_NA,LHS_T> LHS_TYPE ;
 
-	Diff( const LHS_TYPE& lhs_ ) : lhs(lhs_), previous(lhs_[0]) {}
+	Diff( const LHS_TYPE& lhs_ ) : lhs(lhs_), previous(lhs_[0]), previous_index(0) {}
 
 	inline double operator[]( int i ) const {
 		double y = lhs[i+1] ;
+		if( previous_index != i ) previous = lhs[i] ;
 		double res = y - previous ;
 		previous = y ;
+		previous_index = i+1 ;
 		return res ;
 	}
 	inline int size() const { return lhs.size() - 1 ; }
 
 private:
 	const LHS_TYPE& lhs ;
 	mutable double previous ;
+	mutable int previous_index ;
 } ;
 
 template <int RTYPE, typename LHS_T>
@@ -89,19 +100,22 @@ class Diff<RTYPE,false,LHS_T> : public Rcpp::VectorBase< RTYPE, false , Diff<RTY
 	typedef typename Rcpp::VectorBase<RTYPE,false,LHS_T> LHS_TYPE ;
 	typedef typename Rcpp::traits::storage_type<RTYPE>::type STORAGE ;
 
-	Diff( const LHS_TYPE& lhs_ ) : lhs(lhs_) {}
+	Diff( const LHS_TYPE& lhs_ ) : lhs(lhs_), previous(lhs[0]), previous_index(0) {}
 
 	inline STORAGE operator[]( int i ) const {
 		STORAGE y = lhs[i+1] ;
+		if( previous_index != i ) previous = lhs[i] ;
 		STORAGE diff = y - previous ;
 		previous = y ;
+		previous_index = i+1 ;
 		return y - previous ;
 	}
 	inline int size() const { return lhs.size() - 1 ; }
 
 private:
 	const LHS_TYPE& lhs ;
 	mutable STORAGE previous ;
+	mutable int previous_index ;
 } ;
 
 } // sugar

inst/unitTests/cpp/sugar.cpp

@@ -144,6 +144,12 @@ NumericVector runit_diff( NumericVector xx){
     return res ;
 }
 
+// [[Rcpp::export]]
+NumericVector runit_diff_ifelse( LogicalVector pred, NumericVector xx, NumericVector yy){
+    NumericVector res = ifelse( pred, diff(xx), diff(yy) );
+    return res ;
+}
+
 // [[Rcpp::export]]
 List runit_exp( NumericVector xx, IntegerVector yy ){
     return List::create( exp(xx), exp(yy) ) ;

inst/unitTests/runit.sugar.R

@@ -211,9 +211,11 @@ test.sugar.assignment <- function( ){
 }
 
 test.sugar.diff <- function( ){
-	fx <- runit_diff
-	x <- rnorm( 100 )
-	checkEquals( fx(x) , diff(x) )
+    x <- rnorm( 100 )
+    checkEquals( runit_diff(x) , diff(x) ) 
+    y    <- rnorm(100)
+    pred <- sample( c(T,F), 99, replace = TRUE )
+    checkEquals( runit_diff_ifelse(pred, x, y ), ifelse( pred, diff(x), diff(y) ) )
 }
 
 test.sugar.exp <- function( ){