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

consider empty vectors in min() and max() (fixes #883) #884

Merged
merged 1 commit into from Jul 21, 2018

Conversation

Projects
None yet
4 participants
@eddelbuettel
Copy link
Member

eddelbuettel commented Jul 20, 2018

No description provided.

@eddelbuettel

This comment has been minimized.

Copy link
Member Author

eddelbuettel commented Jul 20, 2018

Reviews / thumbs up-or-down welcome! @kevinushey @jjallaire @thirdwing @nathan-russell ...

@kevinushey

This comment has been minimized.

Copy link
Contributor

kevinushey commented Jul 20, 2018

LGTM!

@coatless

This comment has been minimized.

Copy link
Contributor

coatless commented Jul 20, 2018

Why not use R_NaN instead of R_NegInf?

@eddelbuettel

This comment has been minimized.

Copy link
Member Author

eddelbuettel commented Jul 20, 2018

Because of this:

R> v <- numeric(0); c(min(v), max(v))
[1]  Inf -Inf
Warning messages:
1: In min(v) : no non-missing arguments to min; returning Inf
2: In max(v) : no non-missing arguments to max; returning -Inf
R> 

In the integer case it does become NA from the static_cast. Have not tried complex or other vectors.

@eddelbuettel eddelbuettel merged commit 020848a into master Jul 21, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
@eddelbuettel

This comment has been minimized.

Copy link
Member Author

eddelbuettel commented Jul 21, 2018

Running another round of reverse depends now, and only hickup is in BigVAR. It has in file src/ExperimentalBigVARFunctionsX.cpp a function (indented here)

colvec ThreshUpdateOO(const mat& ZZ, double lam,const mat& Y,double eps, List groups, \
       List fullgroups, List compgroups,List M2f_, List eigvalF_, List eigvecF_, \
       colvec& B,int n, int k1) {

with a test

    if(max(groups)==0)

and we now get

ccache g++ -std=c++11 -std=gnu++11 -I"/usr/share/R/include" -DNDEBUG  -I"/usr/local/lib/R/site-library/Rcpp/include" -I"/usr/local/lib/R/site-library/RcppArmadillo/include" -I"/usr/local/lib/R/site-library/Rc\
ppEigen/include"   -DNDEBUG -fpic  -g0 -O3 -Wall -pipe -Wno-unused  -fext-numeric-literals -march=native -c ExperimentalBigVARFunctionsX.cpp -o ExperimentalBigVARFunctionsX.o                                   
In file included from /usr/local/lib/R/site-library/Rcpp/include/Rcpp/sugar/functions/functions.h:50:0,  
                 from /usr/local/lib/R/site-library/Rcpp/include/Rcpp/sugar/sugar.h:31,                  
                 from /usr/local/lib/R/site-library/Rcpp/include/Rcpp.h:74,                              
                 from /usr/local/lib/R/site-library/RcppArmadillo/include/RcppArmadillo.h:34,            
                 from ExperimentalBigVARFunctionsX.cpp:1:                                                
/usr/local/lib/R/site-library/Rcpp/include/Rcpp/sugar/functions/max.h: In instantiation of ‘Rcpp::sugar::Max<RTYPE, NA, T>::operator Rcpp::sugar::Max<RTYPE, NA, T>::STORAGE() const [with int RTYPE = 19; bool NA = true; T = Rcpp::Vector<19>; Rcpp::sugar::Max<RTYPE, NA, T>::STORAGE = SEXPREC*]’:                   
ExperimentalBigVARFunctionsX.cpp:590:20:   required from here                                            
/usr/local/lib/R/site-library/Rcpp/include/Rcpp/sugar/functions/max.h:36:32: error: invalid static_cast from type ‘double’ to type ‘Rcpp::sugar::Max<19, true, Rcpp::Vector<19> >::STORAGE {aka SEXPREC*}’       
             if (n == 0) return(static_cast<STORAGE>(R_NegInf));                                         
                               ~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~                                          
/usr/lib/R/etc/Makeconf:168: recipe for target 'ExperimentalBigVARFunctionsX.o' failed                   
make: *** [ExperimentalBigVARFunctionsX.o] Error 1                                                       
ERROR: compilation failed for package ‘BigVAR’

but I am a little confused how that would have worked before max() over a Rcpp::List comparing to a scalar? Did that ever make sense?

Is this a regression we need to fix?

@kevinushey

This comment has been minimized.

Copy link
Contributor

kevinushey commented Jul 21, 2018

Looks like max() doesn't actually do what one might expect it to do when applied to a List:

#include <Rcpp.h>
using namespace Rcpp;

// [[Rcpp::export]]
SEXP max(List data) {
   return Rcpp::max(data);
}

/*** R
max(list(1, 2, 3, 4, 5))
*/
> Rcpp::sourceCpp('~/scratch/Untitled.cpp')
> max(list(1, 2, 3, 4, 5))
[1] 1

IMHO it's fine that this is a compilation error and perhaps provide a patch (or request a change) from BigVAR.

@kevinushey

This comment has been minimized.

Copy link
Contributor

kevinushey commented Jul 21, 2018

The R level APIs also don't understand lists:

> base::max(list(1, 2, 3, 4, 5))
Error in base::max(list(1, 2, 3, 4, 5)) : 
  invalid 'type' (list) of argument

so IMHO further evidence that not supporting Lists is fine.

@eddelbuettel

This comment has been minimized.

Copy link
Member Author

eddelbuettel commented Jul 21, 2018

Agreed. Did the same / similar test with max() / min() around Rcpp::List for both 0.12.17 and 0.12.18 -- results were incorrect for 0.12.17 as you show, and 0.12.18 fails to compile. With an added as<>() it compiles and still returns the wrong result :)

So I concur: It was sloppy code by us before allowing this compile, but this particular snippet may actually be an issue in BigVAR. I also contacted its maintainer, @wbnicholson.

@wbnicholson

This comment has been minimized.

Copy link

wbnicholson commented Jul 21, 2018

Thanks for letting me know about this issue. I will remove the max call from my package and will re-upload a new version to cran.

@eddelbuettel

This comment has been minimized.

Copy link
Member Author

eddelbuettel commented Jul 21, 2018

Awesome (as also discussed via email).

Turns out that this was the only 'regression' so I will wrap up Rcpp 0.12.18 for CRAN this weekend.

@eddelbuettel eddelbuettel deleted the bugfix/min_max_zero branch Jul 21, 2018

@eddelbuettel eddelbuettel restored the bugfix/min_max_zero branch Jul 23, 2018

@eddelbuettel eddelbuettel deleted the bugfix/min_max_zero branch Jul 23, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.