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

Upstreaming std::tuple<> support #1278

Closed
andrjohns opened this issue Sep 30, 2023 · 10 comments
Closed

Upstreaming std::tuple<> support #1278

andrjohns opened this issue Sep 30, 2023 · 10 comments

Comments

@andrjohns
Copy link
Contributor

andrjohns commented Sep 30, 2023

Reporting an Issue

Over in the cmdstanr package we recently added support for passing std::tuple<> types between c++ and R by treating it as a List, and I was hoping to put together a PR to add it to Rcpp directly (I checked the issues and PRs but I couldn't find anything).

Should these overloads/specifications live in ListOf or VectorBase (or somewhere else completely)?

Also feel completely free to let me know if I'm handling the conversions in a way that's fundamentally wrong/inefficient!

The implementation is below, and just a note that the stan::math::apply is just c++11 implementation of std::apply:

#include <stan/math/prim/functor/apply.hpp>
#include <Rcpp.h>

namespace Rcpp {
  namespace traits {

    template <typename... T>
    class Exporter<std::tuple<T...>> {
      private:
        Rcpp::List list_x;

        template<std::size_t... I>
        auto get_impl(std::index_sequence<I...> i) {
          return std::make_tuple(Rcpp::as<T>(list_x[I].get())...);
        }

      public:
        Exporter(SEXP x) : list_x(x) { }
        std::tuple<T...> get() {
          return get_impl(std::index_sequence_for<T...>{});
        }
    };
  }

  template <typename... T>
  SEXP wrap(const std::tuple<T...>& x) {
    return stan::math::apply([](const auto&... args) {
      return Rcpp::List::create(Rcpp::wrap(args)...);
    }, x);
  }
}
@eddelbuettel
Copy link
Member

Howdy and thanks for suggesting that. We will take a look. It may otherwise be easiest to do this in a small add-on package as e.g. was done in RcppArray (also supporting std::span if C++20 is used).

@eddelbuettel
Copy link
Member

Gee I just realized in horror that I think I never experimented / reached out to @jonclayden here! John, any 'feels' about extending RcppArray with this (pro: it seems to fit; con: it may clutter the package) or whether this should be something on its own (pro: self-contained is good ?; con: clutters package system?).

@jonclayden
Copy link

Very happy to look into merging this into RcppArray, as it does seem like a natural extension and saves creating another small extension package for a similar purpose.

jonclayden added a commit to jonclayden/RcppArray that referenced this issue Nov 3, 2023
@jonclayden
Copy link

The commit referenced above provides a version of the implementation that avoids using helpers from C++14 (index_sequence) and C++17 (apply()), to try and maximise compatibility. Instead it uses a nested helper struct holding references to the tuple and the R list, each "level" of which is responsible for an element of the tuple. I have tests for the as() direction (though not yet for wrap()) and it seems to work. I don't imagine this will be noticeably less efficient, and it compiles with CXX_STD=CXX11, but testing/feedback is very welcome.

@eddelbuettel
Copy link
Member

Sounds good to me, @andrjohns is closer to the metal for a concrete use case. I am sure we can cook up a unit test or two, I always liked how you set up tester packages in RcppArray (and Rcpp has long done similar stuff for parts of its tests).

@jonclayden
Copy link

I've added a tuple client stub package now, and unfortunately wrap() chokes on this assertion, whether my implementation or the version above using apply() is used. The specialisation is declared before including the full Rcpp.h but somehow dispatch isn't working out correctly. I'll need to have another look with a clearer head...

@jonclayden
Copy link

The problem seems to have been including the wrap() implementation within the Rcpp::traits namespace, when it should just be in Rcpp. The Rcpp-extending vignette threw me here, I think, because it seems to indicate that partial specialisations should live in Rcpp::traits (section 2.3), although that does seem surprising – am I misreading it? Anyway, that seems to have been the mistake relative to the version above, now corrected.

@andrjohns I'll add you as a contributor to the RcppArray package, assuming you're happy with that. If the current implementation works for you then I'll put together a release for CRAN.

@eddelbuettel
Copy link
Member

Yes, that documentation / implementation could do with a rewrite. It also throws me for loops at times. So twice the thanks for working through it !!

@jonclayden
Copy link

RcppArray v0.3.0, with the tuple support added, is now on CRAN.

@eddelbuettel
Copy link
Member

We can close this as RcppArray took care of it. Three cheers for that!

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

No branches or pull requests

3 participants