Matrix sugar functions eye, ones, and zeros - part of #564 #566
Conversation
|
You're fast :) That looks rather good from a first glance. |
|
FYI there was a strange error in one of the unit tests when it ran on Travis, but I did not get it on my machine, which is running a nightly build of R: Locally
On Travis
At any rate, I think the behavior on my machine is "correct", but either way the Rcpp function was behaving consistently. |
|
Could be clang vs g++, or old g++ vs new g++. What is your OS and compiler again? |
|
I'm on Debian 8.6, using GCC 4.9.2. I was thinking there may have been something that changed between whatever version of R is running on Travis and what I have which is causing |
|
You could fire up Docker to get a newer g++. I default to 5.4 on Ubuntu 16.04, but my uploads into Debian unstable all have g++-6.2 which is (I think) also the default in Ubuntu 16.10. |
|
Maybe it is "simply" an R versus R-devel thing. On my box (R-release, g++-5.4.0, Ubuntu 16.04) it behaves like Travis: R> library(Rcpp)
R> cppFunction("LogicalMatrix lgl_eye(int n) { return Rcpp::eye(n); }")
R> lgl_eye(3)
[,1] [,2] [,3]
[1,] TRUE FALSE FALSE
[2,] FALSE TRUE FALSE
[3,] FALSE FALSE TRUE
R> storage.mode(lgl_eye(3))
[1] "logical"
R> storage.mode(diag(TRUE, 3, 3))
[1] "double"
R> That is, after pulling your branch and installing, of course. |
|
Hmm that is strange; I would expect the |
|
I really think it is R 3.3.* vs R 3.4.0-to-be: edd@max:~/git/rcpp(feature/pr566)$ RD
R Under development (unstable) (2016-10-14 r71522) -- "Unsuffered Consequences"
Copyright (C) 2016 The R Foundation for Statistical Computing
Platform: x86_64-pc-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.
Natural language support but running in an English locale
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.
R> storage.mode(diag(TRUE, 3, 3))
[1] "logical"
R> and continuing in the same R-devel session: R> library(Rcpp)
R> cppFunction("LogicalMatrix lgl_eye(int n) { return Rcpp::eye(n); }")
R> lgl_eye(3)
[,1] [,2] [,3]
[1,] TRUE FALSE FALSE
[2,] FALSE TRUE FALSE
[3,] FALSE FALSE TRUE
R> storage.mode(lgl_eye(3))
[1] "logical"
R> |
|
Ah, yes -- from the upcoming release notes:
|
|
So you added a unit test from the future. I am in awe 😁 |
|
While we are on the subject, running the full unit test suite did yield this both times: So that may or may not be another R 3.4 issue; but it is potentially something to keep an eye on for the future. |
|
This one from test.ExpressionVector <- function() {
x <- parse(text = "rnrom; rnrom(10); mean(1:10)")
checkEquals(first_el(x), x[1], msg = "RCPP_RETURN_VECTOR (numeric)")
}I don't recall what the story is. An intentional misspelling of |
|
Yes that's that one; I hadn't stopped to look at it before, but for whatever reason it causes an error on my machine and not on Travis. |
|
When I run that here I get so it passes with the usual noise from RUnit (which I should really turn off...) |
|
My runit.sh is a simple edd@max:~/git/rcpp/inst/unitTests(feature/pr566)$ cat ~/bin/runit.sh
#!/bin/sh
set -u
set -e
progname=`basename $0`
options='p:ah?'
usage_and_exit()
{
echo ""
echo "Usage: $progname [-p package[,package2,..]] [-?|-h]"
echo " Run unit test script for R package"
echo ""
echo "Options:"
echo " -p package[,package2,..]] load additional package(s)"
echo " -h show this help"
echo " -a run with 'export RunAllRcppTests=yes'"
echo ""
echo "The RUnit and inline packages are automatically loaded."
echo ""
exit 0
}
while getopts "$options" i
do
case "$i" in
p)
pkg=",$OPTARG"
shift
shift
;;
a)
export RunAllRcppTests="yes"
shift
;;
h|?)
usage_and_exit
;;
esac
done
if [ ! -f $1 ]; then
echo "Error: No file '$1' found"
exit 1
fi
file=`pwd`/$1
#r -i -t -linline,RUnit${pkg} -e"cppfunction <- function(...) cxxfunction(..., plugin=\"Rcpp\"); runTestFile(\"$file\")"
r -i -t -lRUnit${pkg} -e"runTestFile(\"$file\")"
edd@max:~/git/rcpp/inst/unitTests(feature/pr566)$ and I just executed edd@max:~/git/rcpp/inst/unitTests(feature/pr566)$ runit.sh -p Rcpp -a runit.dispatch.R The |
|
Hmm I don't get any errors when running your script; I guess it must have something to do with RStudio. |
|
I get a bunch of warnings when compiling Any chance that warning could be suppressed / disabled somehow? Maybe an |
|
As an aside, I can replicate the This was with R 3.3.1 patched (r71015), so may well be an R-devel change? |
It's a command-line script. RStudio never enters that picture. |
|
@kevinushey I changed the offending lines to use |
|
I am not sure if we can salvage that (templated) ExpressionVector unit test mentioned above. Seems like comparison do not work (illustrating what @kevinushey dug up) R> parse(text="rnorm; rnorm(10); mean(1:10);")[1]
expression(rnorm)
R> parse(text="rnorm; rnorm(10); mean(1:10);")[2]
expression(rnorm(10))
R> parse(text="rnorm; rnorm(10); mean(1:10);")[3]
expression(mean(1:10))
R> eval(parse(text="rnorm; rnorm(10); mean(1:10);")[3])
[1] 5.5
R> library(Rcpp)
R> cppFunction("ExpressionVector foo(ExpressionVector x) { ExpressionVector res(1); res[0] = x[0]; return res;}" )
R> foo(parse(text="rnorm; rnorm(10); mean(1:10);"))
expression(rnorm)So far so good but neither R> foo(parse(text="rnorm; rnorm(10); mean(1:10);")) == parse(text="rnorm; rnorm(10); mean(1:10);")[1]
Error in foo(parse(text = "rnorm; rnorm(10); mean(1:10);")) == parse(text = "rnorm; rnorm(10); mean(1:10);")[1] :
comparison is not allowed for expressions
R> RUnit::checkEquals(foo(parse(text="rnorm; rnorm(10); mean(1:10);")), parse(text="rnorm; rnorm(10); mean(1:10);")[1])
Error in RUnit::checkEquals(foo(parse(text = "rnorm; rnorm(10); mean(1:10);")), :
Attributes: < target is NULL, current is list >
current is not list-like
R> We could just compare to |
|
After looking at this a little bit more, I feel like it was incorrect for that unit test to be using test.ExpressionVector <- function() {
x <- expression(rnorm, rnorm(10), mean(1:10))
checkEquals(first_el(x), x[1], msg = "RCPP_RETURN_VECTOR (numeric)")
}Running the code interactively, x <- parse(text = "rnrom; rnorm(10); mean(1:10)")
x2 <- expression(rnorm, rnorm(10), mean(1:10))
checkEquals(first_el(x), x[1])
# Error in checkEquals(first_el(x), x[1]) :
# Attributes: < target is NULL, current is list >
# current is not list-like
checkEquals(first_el(x2), x2[1])
# [1] TRUE |
|
I like that a lot better. Note that we may as well change the msg there. Do you mind if I just punch that in? |
|
Not at all, and yes I agree, the original (re: current version) also has a typo ("rnrom" instead of "rnorm"), which is probably also worth correcting. |
as discussed in #566, with thanks to Nathan Russell
|
Just pushed it into a fresh baby branch. Take a look and if you nod, and if Travis agrees, I'll merge it in (probably even as squashing merge which is a nice and new-ish GH feature). |
|
Actually we could also have just piled it on to your open PR. Oh well. Six of one, half a dozen of the other. |
|
Dang. Now your PR now longer merges cleanly and we had hoped that having .gitattributes would avoid that. Guess not. |
|
Actually, good news. Doing it 'by hand' in a (local) branch which had pull your master: git merge --ff --no-commit mastergets me: so if you did the equivalent update of our master (in your branch corresponding to the Rcpp master) ... and dang before I was done typing this you already pushed. Well done. |
|
I think maybe the GitHub client just isn't clever enough to apply the nathan@nathan-deb:~/gitrepos/Rcpp$ git merge upstream/master
Auto-merging inst/NEWS.Rd
Auto-merging ChangeLog
Merge made by the 'recursive' strategy.
ChangeLog | 2 ++
inst/NEWS.Rd | 4 ++++
inst/unitTests/runit.dispatch.R | 7 ++++---
3 files changed, 10 insertions(+), 3 deletions(-)Edit: Didn't see you comment until after I that typed up. |
|
Agreed -- maybe the web client is dumber. The real git client handled it for you and me. Will merge once this Travis run is done. All good! |
|
😢 Just started a reverse-dependency check, and it almost immediately barked when AdaptiveSparsity failed because with RcppArmadillo and Rcpp namespaces we now have an ambiguity over Now, that is arguably an error in that package. We'll see how many other bubble up. So far 19 good, 1 bad with a lot more to go. |
|
I'm guessing the author is doing something like using namespace Rcpp;
using namespace arma;And unfortunately, they probably aren't the only person doing this... Edit: Yep nrussell@nrussell ~/tmp/AdaptiveSparsity/src
$ cat CSL.cpp | head
#include "CSL.h"
using namespace Rcpp;
using namespace std;
using namespace arma;
////////////////////// OLS Estimation //////////////////////
// [[Rcpp::export]]
mat armaOLSestim (mat x) {
int m = x.n_rows; |
|
Yes and yes and why I wrote
Just got three more failures. Current score 34 good, 4 bad. |
|
So ... here is what we can do:
I like the |
|
Yes the |
|
Why not something a bit simpler such as renaming the functions to |
|
That could work also; I'm open to either. What do you want to do Dirk? |
|
I hate underscores and find Also, we have one advantage: we do also control RcppArmadillo AND we already control the include order. We could re-define the protective |
|
I think that no matter what the macro guard would have to be inside of #ifdef RCPP_USE_ADDITIONAL_SUGAR_FUNCTIONS
#include <Rcpp/sugar/matrix/eye.h>
#endif Wouldn't that ^^ have to live in |
|
See this one which happens before |
|
Oh I see, so maybe even just adding a simple #define Rcpp__sugar__eye_hbefore the |
|
Right! Now up to us to decide if we want to permanently hide them or not. Maybe say we will flip such a Thoughts? |
|
I guess we can just define that thing now, effectively hide, promised to remove it in a year -- and thereby leaves users an option to add it now or later if they prefer not change their code. |
|
Yes that should work fine. All of the user code that is currently call |
|
Exactly. All we can do is whine, and boy are we good at it ;-) |
|
I'd rather just avoid breaking Rcpp client packages if possible (even if those packages are doing something they shouldn't be doing). Can we move these functions to an alternative place? I'd advocate for placing them within the |
|
I like that option too. |
|
@kevinushey Any reason to prefer putting them in |
|
I initially thought |
|
That's fine; I'll put it in As a side note, for the std::fill(
res.begin(), res.end(),
traits::zero_type<stored_type>()
);But as far as I can tell, |
|
Yes, I think you can. This is the default behaviour here unless you call Edit: Don;t believe a word I am saying^Htyping. As @kevinushey wrote, |
|
The other mechanism for generating vectors / matrices without initialized memory is with the |
This PR adds non-member versions of
eye,ones, andzeros.