Rcpp: A "wish" for future Rcpp version.

[Bijaelo]

This is a question for my own usage, and a continuation of a prior question to R-devel about reimplementing do_docall.

I've gotten to the point, that I am "trying" to translate this into Rcpp. The idea I got was surprisingly simple to implement in theory:

1. Create class ArgumentList, which in almost all aspects is identical to List.
2. Create overloaded function for pairlist(ArgumntList t1); to create the pairlist in a similar fashion to how it is done in do_docall (in R's C api). Function::invoke will then handle installing the function to the front of the returned pairlist and Function can remain unchanged. And one could consider making a wrapper called do_call(Function& func, T& args){func(args);}, but that could lead to confusion when T& is a List and not an ArgumentList.

I hit 2 problems quite quickly, some because of my limited experience with C++ OOP,

1. typedef Vector<VECSXP> ArgumentList seems to be considered identical to typedef Vector<VECSXP> List by the compiler, so it was not "This simple".
2. I am a bit uncertain how one would overload the constructors with type definitions like
   Vector(T size, typename Rcpp::traits::enable_if<traits::is_arithmetic<T>::value, void>::type* = 0)
   in order to pass the latter part to the Vector constructor (and I'm looking into that at the moment, but would also be appreciative with where to start looking. Does it even have to be passed or it is automatic?).

So wrapping this up, an old stackoverflow answer by Romaine mentions the potential need for a do_call function. I am likely missing something, but doesn't this seem like the least intrusive method for doing so? Maybe there'd be a some extra work on as<T>() and wrap (have not gone that deep "yet").

Disclaimer: Currently I'm just looking at overloading Rcpp::pairlist(T& t1) with Rcpp::pairlist(List t1) for my specific needs, but I was going down the path and started working on creating the source for a pull request, until I realized that I am uncertain how the above should be handled appropriately.

Best
Oliver M.

[eddelbuettel]

I am not sure I fully follow what is suggested. But in any event, we are always open to good PRs if you want to pursue this.

As an aside, I also started another zero-depends small repo with some wrappers about C API of R which I currently use in one public source file. If you have particular constructs from R in mind that you could need we may add them there (with a mild preference to the _public_ part of the C API).

[Bijaelo]

After originally posting the question, with 2 wishes, I realized both wishes could be worked around, while I still had a few questions that I were unsuccessful getting through.

A short background:
While developing my own little pet project I have come upon a situation where I would like to replicate do.call, or at least dynamically create and evaluate function a call with an unknown number of parameters (potentially more than the 20 allowed by Function at the moment). It is maybe known I threw in a question to R-devel, while trying to figure out how to implement this (and referred to my solution in a recent stackoverflow answer) but out of curiosity and lack of interest in testing whether it would be accepted on cran, I moved on to looking at a solution that was more in line with the Rcpp code style. And my initial idea was simply to build a call with Function, but as evident from the source it is not plausible in the current implementation. So I moved on to trying to considering how I could write a bit of extension code to allow something like Function F("funname");F(arglist); or do_call(Function, List).

Question 1:
So after reading Romain's answer on stackoverflow, I thought to change my extension to a potential pull-request. But before making the PR, I wanted to check up on the details and be certain that it would be of interest as an addition to Rcpp?

In short I think that the least intrusive and most use friendly solution to translating do_call to Rcpp, would be to allow Functions to accept a single argument list (in addition to the current implementation) and have f(argList) translate f and argList into and evaluate the call, just as it already is for individual arguments. And to avoid this breaking existing code, I am considering the following strategy (also described in the original issue):

1. Create a typedef ArgumentList that works identically to a standard List except for when it is passed as a solo argument to a Function, where it should be parsed as a list of arguments similar to do.call.
   
   
   
   • To allow for this ArgumentList needs to be a typedef from a class derived from Vector, and not identical to typedef ... List, so I would let ArgumentList be a typedef from a class derived from Vector, with all constructors (and operator=s) overloaded to call the base class constructors (and operators).
   
   
2. overload pairlist(T& t1) for T& == ArgumentList&, and have it create the tail-end of the call.
3. Let Function::invoke take care of patching together the function to the call.

As ArgumentList would work identically to a List in all aspects, except for when calling a Function, the user experience would become

SEXP do_call(Function fun, ArgumentList args){
     return fun(args);
}

while from a backend perspective the maintenance would be to ensure ArgumentList has all the constructors from Vector implemented.
As an important side note, most of the code from do_docall can be extracted and used without need for the internal api. Only a single call to PRIMNAME requires internal code, and that is only checking whether the function is .Internal or not. So excluding this part, it could make the base of pairlist(ArgumentList) with only the public C API sprinkled with a bit of Rcpp.

Question 2:
Thoughts on the above strategy? Something for Rcpp or not?
To make it less of a thought experiment I will be adding a pull-request, with the code.

Question 3:
When contributing other packages, contributors are regularly placed in the Description file. Just because I don't see anyone marked "contributor" I am curious how credit is given to contributors in Rcpp.

[eddelbuettel]

In case this is unclear we generally do not recommend unilaterally sending an unsolicited pull request. There is a bit more here.

Contributors are clearly and visibly credited as usual at GitHub as well as in the ChangeLog file.

[Bijaelo]

In case this is unclear we generally do not recommend unilaterally sending an unsolicited pull request. There is a bit more here.

That is clear. It would simply make the strategy more illstrative, rather than linking to a forked repository with the suggested implementation of ArgumentList and the overload for pairlist. :-)

[Enchufa2]

1. typedef Vector<VECSXP> ArgumentList seems to be considered identical to typedef Vector<VECSXP> List by the compiler, so it was not "This simple".

A typedef is just a label. You may call a Vector<VECSXP> in many ways, but it's still the same type. Inheriting from List won't work either, because the child won't have access to private methods from the template Vector class. And anyway, overloading pairlist just for this specific case doesn't seem like a clean approach.

Why not a new separate method in the Function class? The UX would be something like this:

SEXP do_call(Function fun, List args) {
     return fun.doCall(args);
}

The doCall method would accept a list (maybe, optionally, an environment too), and would be reponsible for converting the list to a pairlist and calling invoke.

[Bijaelo]

1. typedef Vector<VECSXP> ArgumentList seems to be considered identical to typedef Vector<VECSXP> List by the compiler, so it was not "This simple".

A typedef is just a label. You may call a Vector<VECSXP> in many ways, but it's still the same type. Inheriting from List won't work either, because the child won't have access to private methods from the template Vector class. And anyway, overloading pairlist just for this specific case doesn't seem like a clean approach.

Why not a new separate method in the Function class? The UX would be something like this:

SEXP do_call(Function fun, List args) {
     return fun.doCall(args);
}

The doCall method would accept a list (maybe, optionally, an environment too), and would be reponsible for converting the list to a pairlist and calling invoke.

I mentioned the first problem, that is why I suggested using a derived class as a "work around" this problem. The point being that the derived class just calls the constructors from the base class, and public members inherited has access to the private members. From my own testing it works like a charm, while of course adding some extra code for the derived class. :-)
Adding an alternative method to the Function class was also an option I toyed around with. Honestly, I just like the cleanness of calling the function operator. But that is my own selfish preference. ;-)
Of course it was also problematic if I went with that approach in my own code, only for it to be rejected as a suggestion. deriving from Vector and overloading pairlist I could "at least" implement and maintain in my own package without depending on it being accepted. But that was before my issue post here. That is likely the reason I moved away from the thought myself.

[Enchufa2]

Here's a possible implementation as a sugar function, with minimal changes to existing codebase. If this is considered a good approach, I can prepare a PR.

[eddelbuettel]

Can you prepare a usage illustration?

[Enchufa2]

For example:

#include <Rcpp.h>
using namespace Rcpp;

// [[Rcpp::export]]
SEXP test(Function fun, List args) {
  return do_call(fun, args);
}

/*** R
test(paste, list(1, 2, 3:5))
*/

[eddelbuettel]

So the point would be to repeatedly call R functions to glue results from a list together?

Meh.

That, to me, is a very standard example of something that can be done in a package outside of Rcpp itself. Receive the function, receive the list, do your thing. Does it have to be inside Rcpp? Is it idiomatic or helpful, or added "because we can" ?

[Enchufa2]

No, the function is invoked once for a pairlist that is constructed from a list of arguments.

[Enchufa2]

In light of the following, probably makes little sense:

#include <Rcpp.h>
using namespace Rcpp;

// [[Rcpp::export]]
SEXP test_internal(Function fun, List args) {
    return do_call(fun, args);
}

// [[Rcpp::export]]
SEXP test_external(Function fun, List args) {
    return Function("do.call")(fun, args);
}

/*** R
x <- as.list(1:1e6)
microbenchmark::microbenchmark(
    test_internal(sum, x),
    test_external(sum, x)
)
# Unit: milliseconds
#                   expr      min       lq     mean   median       uq      max neval cld
#  test_internal(sum, x) 128.3678 145.6932 212.9636 178.9692 255.5325 453.0384   100   a
#  test_external(sum, x) 138.2882 150.8852 202.5377 170.4470 200.2248 461.9199   100   a
*/

[Bijaelo]

I wouldn't expect any massive differences. In your implementation @Enchufa2 there seems to be a slight overhead from calling Rf_cons repeatably. Also, does Rf_cons set the names automatically? Otherwise your suggested sugar implementation would fail on named arguments.

Allocating the pairlist and iteratively setting the arguments (as below and in my linked fork in my prior comment) seems to eliminate the difference (apologies for long code segments, as I felt I was recommended "not" to do a PR). Note that it is more or less a rewritten version from do_docall in R-source:

// do_call.h (without authors and redoing templates)
#ifndef Rcpp__sugar__do_call_h
#define Rcpp__sugar__do_call_h
namespace Rcpp {
  namespace sugar {

    template <bool NA, typename T, typename Function>
      class DoCall {
        public:
          typedef Vector<VECSXP> List; // Vector rather than VectorBase (for .names())

        DoCall(Function fun_, const List& args_, SEXP env_)
        : fun(fun_), args(args_), env(env_) {}

        inline SEXP get() const {
          SEXP arglist, walker; //arglist = tail pointing to the front argument. Walker points to the last part of the tail.
          R_xlen_t n = args.size();
          // need "SEXP names" to allow for both CHARSXP and R_NilValue. (CharacterVector c(R_NilValue); kept complaining)
          SEXP names = args.names();
          PROTECT(arglist = walker = Rf_allocList(n)); // pre-allocate
          if(Rf_length(names) == 0){ // Optimize for "named" and "unnamed" lists
            // unnamed list
            for(R_xlen_t i = 0; i < n; i++){
              SETCAR(walker, args[i]);
              walker = CDR(walker); // Continue walking down the pairlist
            }
          }else{
            // named list
            for(R_xlen_t i = 0; i < n; i++){
              SETCAR(walker, args[i]);
              // Set names tag
              SEXP namei = STRING_ELT(names, i);
              if(namei != R_NilValue){
                SET_TAG(walker, Rf_installTrChar(namei));
              }
              walker = CDR(walker); // Continue walking down the pairlist
            }
          }
          UNPROTECT(1); // ~ names
          return fun.invoke(arglist, env); // Invoke using the front of the tail
        }

         private:
          Function fun;
        const List& args;
        SEXP env;

      };

  } // sugar

  template <bool NA, typename T, typename Function>
    SEXP do_call(Function fun, const Rcpp::VectorBase<VECSXP,NA,T>& args, SEXP env = R_GlobalEnv){
      return sugar::DoCall<NA,T,Function>(fun, args, env).get();
    }

} // Rcpp

#endif

with benchmarks:

sourceCpp(code = 
'
#include <Rcpp.h>
using namespace Rcpp;

// [[Rcpp::export]]
SEXP test_internal(Function fun, List args){
  return Rcpp::do_call(fun, args);
}
// [[Rcpp::export]]
SEXP test_external(Function fun, List args) {
    return Function("do.call")(fun, args);
}
')

x <- as.list(1:1e5)
microbenchmark::microbenchmark(
    test_internal(sum, x),
    test_external(sum, x), times = 1000
)
#Unit: milliseconds
#                 expr   min    lq     mean median    uq     max neval cld
#  test_internal(sum, x) 14.8841 17.34250 20.45252 18.58750 20.83385 163.2438  1000   a
# test_external(sum, x) 14.3831 17.31585 20.84080 18.68835 21.28855 171.0313  1000   a

x <- list(data = mtcars, formula = mpg ~ hp)
microbenchmark::microbenchmark(
  test_internal(lm, x),
  test_external(lm, x), times = 1e4
)
#Unit: microseconds
#                 expr   min    lq     mean median    uq     max neval cld
# test_internal(lm, x) 603.5 630.9 759.6432 644.55 713.3 17579.9 10000   a
# test_external(lm, x) 604.6 635.3 761.1980 649.70 729.6  8523.9 10000   a

identical(test_internal(lm, x), test_external(lm, x))
#[1] TRUE

Note i used a slightly faster evaluation, as I am only really interested in the "difference" between going through R and extra time. Also that the 2 times vary back and forth whether they are faster or slower when the benchmarks are repeated.

Whether this is worth it, I would say is not really an argument of whether it is "faster" however. Rather that it is not "slower" and more convenient and intuitive than f("do.call")(fun, args).

[Enchufa2]

I don't see the inconvenience of setting:

Function do_call("do.call");

and then you can use do_call in just the same way. So given that this is not significantly slower, I don't see the point.

[Bijaelo]

I don't see the inconvenience of setting:

Function do_call("do.call");

and then you can use do_call in just the same way. So given that this is not significantly slower, I don't see the point.

That is a fair argument. In my specific case, I would be performing a call multiple (potentially hundreds) of times with one or more arguments changing between calls. But even here the difference is small (potentially 4 % for unnamed, and somewhere near 0-1 % for named arguments). Regardless, I'm thankful for both of you hearing me out on my suggestion, and I'll keep my wish for no other reason than "I find it more convenient". :-)

Code for quoted numbers:

sourceCpp(code = 
'
#include <Rcpp.h>
using namespace Rcpp;

// [[Rcpp::export]]
SEXP test_internal(Function fun, List args){
for(int i = 0; i < 999; i++){
  Rcpp::do_call(fun, args);
}
  return Rcpp::do_call(fun, args);
}
// [[Rcpp::export]]
SEXP test_external(Function fun, List args) {
   Function do_callo("do.call");
   for(int i = 0; i < 999; i++){
    do_callo(fun, args);
   }
   return do_callo(fun, args);
}
// [[Rcpp::export]]
SEXP test_external_dumb(Function fun, List args) {
   for(int i = 0; i < 999; i++){
    Function("do.call")(fun, args);
   }
   return Function("do.call")(fun, args);
}
')
x <- as.list(1:100)
microbenchmark::microbenchmark(
    test_internal(sum, x),
    test_external(sum, x),
    test_external_dumb(sum, x)
)
# ~ 4 % for unnamed
#Unit: milliseconds
#                       expr     min       lq     mean   median      uq      max neval cld
#      test_internal(sum, x) 25.7494 31.06940 37.01126 32.82840 38.1576  83.5404   100   a
#      test_external(sum, x) 25.8965 31.45770 38.43972 34.31090 43.1511 100.4506   100   a
# test_external_dumb(sum, x) 27.2760 31.34975 40.49221 34.92625 42.1710 166.0161   100   a

x <- list(data = mtcars, formula = mpg ~ hp)
microbenchmark::microbenchmark(
  test_internal(lm, x),
  test_external(lm, x),
  test_external_dumb(lm, x)
)
# ~ 1 % for named
#Unit: milliseconds
#                      expr      min        lq     mean   median       uq      max neval cld
#      test_internal(lm, x) 918.1778  996.4706 1151.041 1108.009 1215.755 2428.933   100   a
#      test_external(lm, x) 931.1719  991.5943 1134.715 1122.249 1224.516 1583.251   100   a
# test_external_dumb(lm, x) 930.6835 1001.5802 1232.792 1134.145 1236.734 4037.689   100   a

[eddelbuettel]

Yes, those times are essentially ties so the upside of adding extra code is limited---it in essence only adds complexity for us to maintain. So some cost at limited benefit.

But let me say that I appreciate that you dug through the code and both a) made an honest proposal and b) are not (too ;-) offended that we have reservations about it. Now, if you look closely at the Rcpp source you will see that we do in fact resort to Rcpp::Function() a few times because a) we can [ we are always called from R ] and b) replacing some things is either impossible or an unneccessarily large amount of work.

And there could well be some work worth doing to improve things around the Function class and call.

Years ago when Rcpp11 split off and made a lot of statements about all the things it did better (because it could, being a refactoring / reimplementation and all that). One aspect was varargs and added flexibility from C++11 which, at the time, was still 'out of reach' for most/all CRAN builds. These days C++11 is standard, C++14 wil be standard under R 4.1.0 come April and C++17 is possible (all with some caveats as we still have users on both older R and older compiler settings due to CentOS and all that). So if you wanted to dig a little and make improvements, that may be a venue to go! (And as you know now, we like that in measured steps. We do have a responsibility to the over 2200 reverse dependencies on CRAN alone and nobody-knows-how-many other uses "in the wild".)

[Enchufa2]

No problem. At the very least, it has been a nice exercise for me to get to better know all the sugar part, which is pretty clever, BTW.

[Bijaelo]

Never offended about these kind of things, as long as you are not shy for a discussion back and forth. ;-)
I've been spenting this Covid lockdown brushing up on me (very!) rusty C++ skills, and writing a package with backend in C++ seemed like the most natural way for me to do so. And from my experience the best learning comes from digging through Source code, documentation (cppreference) and stackoverflow and then following it by trying it, and having ones errors visualized by those smarter within the field.
And luckily Rcpp is much easier to dig through than R C-api (I mean, it isn't even mentioned exactly which files are exposed and which aren't!). Also the documentation is glorious by comparison!

Thank you @eddelbuettel, it is not unlikely I'll be spenting an unknown amount of hours digging through the source code (at least until Covid lockdown ends), and if I come up with something, I'll be sure to mention this later. And Thanks @Enchufa2, you opened my eyes to sugar in the future, and your example was just amazing. Although it will cause me to go back and further clean some of my own code.

One note: I would suggest updating the contributing.md, such that it points to the correct unit-test folder and mentions that Rcpp has to comply with C++11 rather than C++98. I do not think this has to be "guarded" anymore (as it mentions)?

[eddelbuettel]

You may have misunderstood me. I quoted what R Core says in the manual (current: C++11, soon C++14) and then added _that we are aware of a large number of existing older installations leading us to be a little more careful in minimal requirements_. To be more plain, C++11 can now (thankfully) be used but we are not willing to make that the floor. I have that "problem" with RcppArmadillo where C++11 is now a requirement, and I do get a (slow but steady) stream of bug reports. I wish those people updated away from CentOS systems build in 1848 but they don't. Nothing we can do about that, sadly. So given how much Rcpp is a building block for work by other people, or goal is not to erect more road blocks for them. We have other fun side projects with newer code bases...

[Bijaelo]

Ah makes sense!
Then it is really only the unit test link that seems to be broken.

[eddelbuettel]

I am sorry: which link, and broken in respect to what?

[Bijaelo]

contributing.md

Line 33,

[unit tests](https://github.com/RcppCore/Rcpp/tree/master/inst/unitTests).

I am assuming it should now point to

[unit tests](https://github.com/RcppCore/Rcpp/tree/master/inst/tinytest).

:-)

[eddelbuettel]

Excellent catch, I had looked on the file within .github/ :) Now fixed in 5530578b.